This patch set brings atomic operation to i.MX's PWMv2 driver.
This work has been supported and suggested by Boris Brezillon [1] and
Stefan Agner, by showing how simple the transition could be :-).
It has been divided into several steps:
- Separate PWMv1 commits from "generic" and non atomic PWM code.
NOTE: Since I do not have board with PWMv1, I would like to ask somebody
for testing
- Move some imx_config_v2 code to separate functions
- Provide PWM atomic implementation (the ->apply() driver) in a single patch
for better readability.
- Remove redundant PWM code (disable, enable, config callbacks)
- Clean up the driver infrastructure
- Provide "polarity_supported" flag to indicate support for polarity
inversion
This work should be applied on top of following commits:
http://patchwork.ozlabs.org/patch/679706/
http://patchwork.ozlabs.org/patch/679707/
http://patchwork.ozlabs.org/patch/679680/
Test HW:
--------
This patch set has been tested on i.MX6q board with vanilla 4.7 kernel.
It applies clearly on 4.9-rcX SHA1: 0c2b6dc4fd4fa13796b319aae969a009f03222c6
The PWM operation has been tested with pwm_bl backlight driver by changing
its brightness.
[1]: http://patchwork.ozlabs.org/patch/685402/
Lukasz Majewski (6):
pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm
operation
pwm: imx: Move PWMv2 software reset code to a separate function
pwm: imx: Move PWMv2 wait for fifo slot code to a separate function
pwm: imx: Provide atomic PWM support for IMXv2 PWM
pwm: imx: Remove redundant IMX PWMv2 code
pwm: imx: Introduce "polarity_supported" flag to PWMv2 driver
drivers/pwm/pwm-imx.c | 262 ++++++++++++++++++++++----------------------------
1 file changed, 114 insertions(+), 148 deletions(-)
--
2.1.4
The code has been rewritten to remove "generic" calls to
imx_pwm_{enable|disable|config}.
Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
implementation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
1 file changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index c37d223..83e43d5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -66,8 +66,6 @@ struct imx_chip {
static int imx_pwm_config_v1(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
- struct imx_chip *imx = to_imx_chip(chip);
-
/*
* The PWM subsystem allows for exact frequencies. However,
* I cannot connect a scope on my device to the PWM line and
@@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
* both the prescaler (/1 .. /128) and then by CLKSEL
* (/2 .. /16).
*/
+ struct imx_chip *imx = to_imx_chip(chip);
u32 max = readl(imx->mmio_base + MX1_PWMP);
u32 p = max * duty_ns / period_ns;
+ int ret;
+
+ ret = clk_prepare_enable(imx->clk_ipg);
+ if (ret)
+ return ret;
+
writel(max - p, imx->mmio_base + MX1_PWMS);
+ clk_disable_unprepare(imx->clk_ipg);
+
return 0;
}
-static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct imx_chip *imx = to_imx_chip(chip);
+ int ret;
u32 val;
+ ret = clk_prepare_enable(imx->clk_ipg);
+ if (ret)
+ return ret;
+
val = readl(imx->mmio_base + MX1_PWMC);
+ val |= MX1_PWMC_EN;
+ writel(val, imx->mmio_base + MX1_PWMC);
- if (enable)
- val |= MX1_PWMC_EN;
- else
- val &= ~MX1_PWMC_EN;
+ clk_disable_unprepare(imx->clk_per);
+
+ return 0;
+}
+
+static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ u32 val;
+
+ val = readl(imx->mmio_base + MX1_PWMC);
+ val &= ~MX1_PWMC_EN;
writel(val, imx->mmio_base + MX1_PWMC);
+
+ clk_disable_unprepare(imx->clk_per);
}
static int imx_pwm_config_v2(struct pwm_chip *chip,
@@ -269,9 +293,9 @@ static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
}
static struct pwm_ops imx_pwm_ops_v1 = {
- .enable = imx_pwm_enable,
- .disable = imx_pwm_disable,
- .config = imx_pwm_config,
+ .enable = imx_pwm_enable_v1,
+ .disable = imx_pwm_disable_v1,
+ .config = imx_pwm_config_v1,
.owner = THIS_MODULE,
};
@@ -291,8 +315,6 @@ struct imx_pwm_data {
};
static struct imx_pwm_data imx_pwm_data_v1 = {
- .config = imx_pwm_config_v1,
- .set_enable = imx_pwm_set_enable_v1,
.pwm_ops = &imx_pwm_ops_v1,
};
--
2.1.4
This commit provides apply() callback implementation for IMX's PWMv2.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 79 insertions(+)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index f3577c5..df25379 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -171,6 +171,84 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
}
}
+static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ unsigned long period_cycles, duty_cycles, prescale;
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct pwm_state cstate;
+ unsigned long long c;
+ u32 cr = 0;
+ int ret;
+
+ ret = clk_prepare_enable(imx->clk_ipg);
+ if (ret)
+ return ret;
+
+ pwm_get_state(pwm, &cstate);
+
+ c = clk_get_rate(imx->clk_per);
+ c *= state->period;
+
+ do_div(c, 1000000000);
+ period_cycles = c;
+
+ prescale = period_cycles / 0x10000 + 1;
+
+ period_cycles /= prescale;
+ c = (unsigned long long)period_cycles * state->duty_cycle;
+ do_div(c, state->period);
+ duty_cycles = c;
+
+ /*
+ * according to imx pwm RM, the real period value should be
+ * PERIOD value in PWMPR plus 2.
+ */
+ if (period_cycles > 2)
+ period_cycles -= 2;
+ else
+ period_cycles = 0;
+
+ /* Enable the clock if the PWM is being enabled. */
+ if (state->enabled && !cstate.enabled) {
+ ret = clk_prepare_enable(imx->clk_per);
+ if (ret)
+ return ret;
+ }
+
+ /*
+ * Wait for a free FIFO slot if the PWM is already enabled, and flush
+ * the FIFO if the PWM was disabled and is about to be enabled.
+ */
+ if (cstate.enabled)
+ imx_pwm_wait_fifo_slot(chip, pwm);
+ else if (state->enabled)
+ imx_pwm_sw_reset(chip);
+
+ writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+ writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+
+ cr |= MX3_PWMCR_PRESCALER(prescale) |
+ MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+ MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
+
+ if (state->enabled)
+ cr |= MX3_PWMCR_EN;
+
+ if (state->polarity == PWM_POLARITY_INVERSED)
+ cr |= MX3_PWMCR_POUTC;
+
+ writel(cr, imx->mmio_base + MX3_PWMCR);
+
+ /* Disable the clock if the PWM is being disabled. */
+ if (!state->enabled && cstate.enabled)
+ clk_disable_unprepare(imx->clk_per);
+
+ clk_disable_unprepare(imx->clk_ipg);
+
+ return 0;
+}
+
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -322,6 +400,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
.disable = imx_pwm_disable,
.set_polarity = imx_pwm_set_polarity,
.config = imx_pwm_config,
+ .apply = imx_pwm_apply_v2,
.owner = THIS_MODULE,
};
--
2.1.4
The code providing functionality surpassed by the PWM atomic is not needed
anymore and hence can be removed.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 155 --------------------------------------------------
1 file changed, 155 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index df25379..02d3dfd 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -55,10 +55,6 @@ struct imx_chip {
void __iomem *mmio_base;
struct pwm_chip chip;
-
- int (*config)(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns);
- void (*set_enable)(struct pwm_chip *chip, bool enable);
};
#define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
@@ -249,145 +245,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
-static int imx_pwm_config_v2(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- unsigned long long c;
- unsigned long period_cycles, duty_cycles, prescale;
- bool enable = pwm_is_enabled(pwm);
- u32 cr;
-
- /*
- * i.MX PWMv2 has a 4-word sample FIFO.
- * In order to avoid FIFO overflow issue, we do software reset
- * to clear all sample FIFO if the controller is disabled or
- * wait for a full PWM cycle to get a relinquished FIFO slot
- * when the controller is enabled and the FIFO is fully loaded.
- */
- if (enable)
- imx_pwm_wait_fifo_slot(chip, pwm);
- else
- imx_pwm_sw_reset(chip);
-
- c = clk_get_rate(imx->clk_per);
- c = c * period_ns;
- do_div(c, 1000000000);
- period_cycles = c;
-
- prescale = period_cycles / 0x10000 + 1;
-
- period_cycles /= prescale;
- c = (unsigned long long)period_cycles * duty_ns;
- do_div(c, period_ns);
- duty_cycles = c;
-
- /*
- * according to imx pwm RM, the real period value should be
- * PERIOD value in PWMPR plus 2.
- */
- if (period_cycles > 2)
- period_cycles -= 2;
- else
- period_cycles = 0;
-
- writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
- writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
- cr = MX3_PWMCR_PRESCALER(prescale) |
- MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
- MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
- if (enable)
- cr |= MX3_PWMCR_EN;
-
- if (pwm->args.polarity == PWM_POLARITY_INVERSED)
- cr |= MX3_PWMCR_POUTC;
-
- writel(cr, imx->mmio_base + MX3_PWMCR);
-
- return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- u32 val;
-
- val = readl(imx->mmio_base + MX3_PWMCR);
-
- if (enable)
- val |= MX3_PWMCR_EN;
- else
- val &= ~MX3_PWMCR_EN;
-
- writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- int ret;
-
- ret = clk_prepare_enable(imx->clk_ipg);
- if (ret)
- return ret;
-
- ret = imx->config(chip, pwm, duty_ns, period_ns);
-
- clk_disable_unprepare(imx->clk_ipg);
-
- return ret;
-}
-
-static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- int ret;
-
- ret = clk_prepare_enable(imx->clk_per);
- if (ret)
- return ret;
-
- imx->set_enable(chip, true);
-
- return 0;
-}
-
-static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct imx_chip *imx = to_imx_chip(chip);
-
- imx->set_enable(chip, false);
-
- clk_disable_unprepare(imx->clk_per);
-}
-
-static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
- enum pwm_polarity polarity)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- u32 val;
-
- if (polarity == pwm->args.polarity)
- return 0;
-
- val = readl(imx->mmio_base + MX3_PWMCR);
-
- if (polarity == PWM_POLARITY_INVERSED)
- val |= MX3_PWMCR_POUTC;
- else
- val &= ~MX3_PWMCR_POUTC;
-
- writel(val, imx->mmio_base + MX3_PWMCR);
-
- dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
- polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
-
- return 0;
-}
-
static struct pwm_ops imx_pwm_ops_v1 = {
.enable = imx_pwm_enable_v1,
.disable = imx_pwm_disable_v1,
@@ -396,18 +253,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
};
static struct pwm_ops imx_pwm_ops_v2 = {
- .enable = imx_pwm_enable,
- .disable = imx_pwm_disable,
- .set_polarity = imx_pwm_set_polarity,
- .config = imx_pwm_config,
.apply = imx_pwm_apply_v2,
.owner = THIS_MODULE,
};
struct imx_pwm_data {
- int (*config)(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns);
- void (*set_enable)(struct pwm_chip *chip, bool enable);
struct pwm_ops *pwm_ops;
};
@@ -416,8 +266,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
};
static struct imx_pwm_data imx_pwm_data_v2 = {
- .config = imx_pwm_config_v2,
- .set_enable = imx_pwm_set_enable_v2,
.pwm_ops = &imx_pwm_ops_v2,
};
@@ -476,9 +324,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
if (IS_ERR(imx->mmio_base))
return PTR_ERR(imx->mmio_base);
- imx->config = data->config;
- imx->set_enable = data->set_enable;
-
ret = pwmchip_add(&imx->chip);
if (ret < 0)
return ret;
--
2.1.4
The need for set_polarity() function has been removed by implementing
PWM atomic support (apply() callback).
To indicate that the PWMv2 supports polarity inversion, new flag -
"polarity_supported" has been introduced.
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 02d3dfd..be3034d 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
};
struct imx_pwm_data {
+ bool polarity_supported;
struct pwm_ops *pwm_ops;
};
@@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
};
static struct imx_pwm_data imx_pwm_data_v2 = {
+ .polarity_supported = true,
.pwm_ops = &imx_pwm_ops_v2,
};
@@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
imx->chip.base = -1;
imx->chip.npwm = 1;
imx->chip.can_sleep = true;
- if (data->pwm_ops->set_polarity) {
+ if (data->polarity_supported) {
dev_dbg(&pdev->dev, "PWM supports output inversion\n");
imx->chip.of_xlate = of_pwm_xlate_with_flags;
imx->chip.of_pwm_n_cells = 3;
--
2.1.4
The software reset code has been extracted from imx_pwm_config_v2 function
and moved to new one - imx_pwm_sw_reset().
This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 83e43d5..fac5c93 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(imx->clk_per);
}
+static void imx_pwm_sw_reset(struct pwm_chip *chip)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct device *dev = chip->dev;
+ int wait_count = 0;
+ u32 cr;
+
+ writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
+ do {
+ usleep_range(200, 1000);
+ cr = readl(imx->mmio_base + MX3_PWMCR);
+ } while ((cr & MX3_PWMCR_SWR) &&
+ (wait_count++ < MX3_PWM_SWR_LOOP));
+
+ if (cr & MX3_PWMCR_SWR)
+ dev_warn(dev, "software reset timeout\n");
+}
+
+
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
unsigned long period_cycles, duty_cycles, prescale;
unsigned int period_ms;
bool enable = pwm_is_enabled(pwm);
- int wait_count = 0, fifoav;
+ int fifoav;
u32 cr, sr;
/*
@@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
dev_warn(dev, "there is no free FIFO slot\n");
}
- } else {
- writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
- do {
- usleep_range(200, 1000);
- cr = readl(imx->mmio_base + MX3_PWMCR);
- } while ((cr & MX3_PWMCR_SWR) &&
- (wait_count++ < MX3_PWM_SWR_LOOP));
-
- if (cr & MX3_PWMCR_SWR)
- dev_warn(dev, "software reset timeout\n");
- }
+ } else
+ imx_pwm_sw_reset(chip);
c = clk_get_rate(imx->clk_per);
c = c * period_ns;
--
2.1.4
The code, which waits for fifo slot, has been extracted from
imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().
This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
drivers/pwm/pwm-imx.c | 42 +++++++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 17 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index fac5c93..f3577c5 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -149,18 +149,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
dev_warn(dev, "software reset timeout\n");
}
+static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct device *dev = chip->dev;
+ unsigned int period_ms;
+ int fifoav;
+ u32 sr;
+
+ sr = readl(imx->mmio_base + MX3_PWMSR);
+ fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+ if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+ period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+ NSEC_PER_MSEC);
+ msleep(period_ms);
+
+ sr = readl(imx->mmio_base + MX3_PWMSR);
+ if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+ dev_warn(dev, "there is no free FIFO slot\n");
+ }
+}
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
struct imx_chip *imx = to_imx_chip(chip);
- struct device *dev = chip->dev;
unsigned long long c;
unsigned long period_cycles, duty_cycles, prescale;
- unsigned int period_ms;
bool enable = pwm_is_enabled(pwm);
- int fifoav;
- u32 cr, sr;
+ u32 cr;
/*
* i.MX PWMv2 has a 4-word sample FIFO.
@@ -169,19 +187,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
* wait for a full PWM cycle to get a relinquished FIFO slot
* when the controller is enabled and the FIFO is fully loaded.
*/
- if (enable) {
- sr = readl(imx->mmio_base + MX3_PWMSR);
- fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
- if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
- period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
- NSEC_PER_MSEC);
- msleep(period_ms);
-
- sr = readl(imx->mmio_base + MX3_PWMSR);
- if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
- dev_warn(dev, "there is no free FIFO slot\n");
- }
- } else
+ if (enable)
+ imx_pwm_wait_fifo_slot(chip, pwm);
+ else
imx_pwm_sw_reset(chip);
c = clk_get_rate(imx->clk_per);
--
2.1.4
On Sun, 23 Oct 2016 23:45:41 +0200
Lukasz Majewski <[email protected]> wrote:
> The code has been rewritten to remove "generic" calls to
> imx_pwm_{enable|disable|config}.
>
> Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> implementation.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index c37d223..83e43d5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -66,8 +66,6 @@ struct imx_chip {
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> - struct imx_chip *imx = to_imx_chip(chip);
> -
> /*
> * The PWM subsystem allows for exact frequencies. However,
> * I cannot connect a scope on my device to the PWM line and
> @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> * both the prescaler (/1 .. /128) and then by CLKSEL
> * (/2 .. /16).
> */
> + struct imx_chip *imx = to_imx_chip(chip);
> u32 max = readl(imx->mmio_base + MX1_PWMP);
> u32 p = max * duty_ns / period_ns;
> + int ret;
> +
> + ret = clk_prepare_enable(imx->clk_ipg);
> + if (ret)
> + return ret;
> +
> writel(max - p, imx->mmio_base + MX1_PWMS);
>
> + clk_disable_unprepare(imx->clk_ipg);
> +
> return 0;
> }
>
> -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct imx_chip *imx = to_imx_chip(chip);
> + int ret;
> u32 val;
>
> + ret = clk_prepare_enable(imx->clk_ipg);
> + if (ret)
> + return ret;
> +
> val = readl(imx->mmio_base + MX1_PWMC);
> + val |= MX1_PWMC_EN;
> + writel(val, imx->mmio_base + MX1_PWMC);
>
> - if (enable)
> - val |= MX1_PWMC_EN;
> - else
> - val &= ~MX1_PWMC_EN;
> + clk_disable_unprepare(imx->clk_per);
> +
> + return 0;
> +}
> +
> +static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + u32 val;
> +
> + val = readl(imx->mmio_base + MX1_PWMC);
> + val &= ~MX1_PWMC_EN;
>
> writel(val, imx->mmio_base + MX1_PWMC);
> +
> + clk_disable_unprepare(imx->clk_per);
> }
>
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> @@ -269,9 +293,9 @@ static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> }
>
> static struct pwm_ops imx_pwm_ops_v1 = {
> - .enable = imx_pwm_enable,
> - .disable = imx_pwm_disable,
> - .config = imx_pwm_config,
> + .enable = imx_pwm_enable_v1,
> + .disable = imx_pwm_disable_v1,
> + .config = imx_pwm_config_v1,
> .owner = THIS_MODULE,
> };
>
> @@ -291,8 +315,6 @@ struct imx_pwm_data {
> };
>
> static struct imx_pwm_data imx_pwm_data_v1 = {
> - .config = imx_pwm_config_v1,
> - .set_enable = imx_pwm_set_enable_v1,
> .pwm_ops = &imx_pwm_ops_v1,
> };
>
On Sun, 23 Oct 2016 23:45:42 +0200
Lukasz Majewski <[email protected]> wrote:
> The software reset code has been extracted from imx_pwm_config_v2 function
> and moved to new one - imx_pwm_sw_reset().
>
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
Just a nit below ;).
BTW, can't you just merge path 2 and 3?
> ---
> drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 83e43d5..fac5c93 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> clk_disable_unprepare(imx->clk_per);
> }
>
> +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + struct device *dev = chip->dev;
> + int wait_count = 0;
> + u32 cr;
> +
> + writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> + do {
> + usleep_range(200, 1000);
> + cr = readl(imx->mmio_base + MX3_PWMCR);
> + } while ((cr & MX3_PWMCR_SWR) &&
> + (wait_count++ < MX3_PWM_SWR_LOOP));
> +
> + if (cr & MX3_PWMCR_SWR)
> + dev_warn(dev, "software reset timeout\n");
> +}
> +
> +
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> unsigned long period_cycles, duty_cycles, prescale;
> unsigned int period_ms;
> bool enable = pwm_is_enabled(pwm);
> - int wait_count = 0, fifoav;
> + int fifoav;
> u32 cr, sr;
>
> /*
> @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> dev_warn(dev, "there is no free FIFO slot\n");
> }
> - } else {
> - writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> - do {
> - usleep_range(200, 1000);
> - cr = readl(imx->mmio_base + MX3_PWMCR);
> - } while ((cr & MX3_PWMCR_SWR) &&
> - (wait_count++ < MX3_PWM_SWR_LOOP));
> -
> - if (cr & MX3_PWMCR_SWR)
> - dev_warn(dev, "software reset timeout\n");
> - }
> + } else
> + imx_pwm_sw_reset(chip);
} else {
imx_pwm_sw_reset(chip);
}
>
> c = clk_get_rate(imx->clk_per);
> c = c * period_ns;
On Sun, 23 Oct 2016 23:45:43 +0200
Lukasz Majewski <[email protected]> wrote:
> The code, which waits for fifo slot, has been extracted from
> imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().
>
> This change reduces the overall size of imx_pwm_config_v2() and prepares
> it for atomic PWM operation.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 42 +++++++++++++++++++++++++-----------------
> 1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index fac5c93..f3577c5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -149,18 +149,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
> dev_warn(dev, "software reset timeout\n");
> }
>
> +static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> + struct pwm_device *pwm)
> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + struct device *dev = chip->dev;
> + unsigned int period_ms;
> + int fifoav;
> + u32 sr;
> +
> + sr = readl(imx->mmio_base + MX3_PWMSR);
> + fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> + if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> + period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
> + NSEC_PER_MSEC);
> + msleep(period_ms);
> +
> + sr = readl(imx->mmio_base + MX3_PWMSR);
> + if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> + dev_warn(dev, "there is no free FIFO slot\n");
> + }
> +}
>
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> struct imx_chip *imx = to_imx_chip(chip);
> - struct device *dev = chip->dev;
> unsigned long long c;
> unsigned long period_cycles, duty_cycles, prescale;
> - unsigned int period_ms;
> bool enable = pwm_is_enabled(pwm);
> - int fifoav;
> - u32 cr, sr;
> + u32 cr;
>
> /*
> * i.MX PWMv2 has a 4-word sample FIFO.
> @@ -169,19 +187,9 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
> * wait for a full PWM cycle to get a relinquished FIFO slot
> * when the controller is enabled and the FIFO is fully loaded.
> */
> - if (enable) {
> - sr = readl(imx->mmio_base + MX3_PWMSR);
> - fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> - if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> - period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
> - NSEC_PER_MSEC);
> - msleep(period_ms);
> -
> - sr = readl(imx->mmio_base + MX3_PWMSR);
> - if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> - dev_warn(dev, "there is no free FIFO slot\n");
> - }
> - } else
> + if (enable)
> + imx_pwm_wait_fifo_slot(chip, pwm);
> + else
> imx_pwm_sw_reset(chip);
>
> c = clk_get_rate(imx->clk_per);
On Sun, 23 Oct 2016 23:45:44 +0200
Lukasz Majewski <[email protected]> wrote:
> This commit provides apply() callback implementation for IMX's PWMv2.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 79 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index f3577c5..df25379 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -171,6 +171,84 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> }
> }
>
> +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + unsigned long period_cycles, duty_cycles, prescale;
> + struct imx_chip *imx = to_imx_chip(chip);
> + struct pwm_state cstate;
> + unsigned long long c;
> + u32 cr = 0;
> + int ret;
> +
> + ret = clk_prepare_enable(imx->clk_ipg);
> + if (ret)
> + return ret;
> +
> + pwm_get_state(pwm, &cstate);
> +
> + c = clk_get_rate(imx->clk_per);
> + c *= state->period;
> +
> + do_div(c, 1000000000);
> + period_cycles = c;
> +
> + prescale = period_cycles / 0x10000 + 1;
> +
> + period_cycles /= prescale;
> + c = (unsigned long long)period_cycles * state->duty_cycle;
> + do_div(c, state->period);
> + duty_cycles = c;
> +
> + /*
> + * according to imx pwm RM, the real period value should be
> + * PERIOD value in PWMPR plus 2.
> + */
> + if (period_cycles > 2)
> + period_cycles -= 2;
> + else
> + period_cycles = 0;
> +
> + /* Enable the clock if the PWM is being enabled. */
> + if (state->enabled && !cstate.enabled) {
> + ret = clk_prepare_enable(imx->clk_per);
> + if (ret)
> + return ret;
> + }
> +
> + /*
> + * Wait for a free FIFO slot if the PWM is already enabled, and flush
> + * the FIFO if the PWM was disabled and is about to be enabled.
> + */
> + if (cstate.enabled)
> + imx_pwm_wait_fifo_slot(chip, pwm);
> + else if (state->enabled)
> + imx_pwm_sw_reset(chip);
> +
> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> + writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> + cr |= MX3_PWMCR_PRESCALER(prescale) |
> + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> +
> + if (state->enabled)
> + cr |= MX3_PWMCR_EN;
> +
> + if (state->polarity == PWM_POLARITY_INVERSED)
> + cr |= MX3_PWMCR_POUTC;
> +
> + writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> + /* Disable the clock if the PWM is being disabled. */
> + if (!state->enabled && cstate.enabled)
> + clk_disable_unprepare(imx->clk_per);
> +
> + clk_disable_unprepare(imx->clk_ipg);
> +
> + return 0;
> +}
> +
> static int imx_pwm_config_v2(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -322,6 +400,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> .disable = imx_pwm_disable,
> .set_polarity = imx_pwm_set_polarity,
> .config = imx_pwm_config,
> + .apply = imx_pwm_apply_v2,
> .owner = THIS_MODULE,
> };
>
On Sun, 23 Oct 2016 23:45:45 +0200
Lukasz Majewski <[email protected]> wrote:
> The code providing functionality surpassed by the PWM atomic is not needed
> anymore and hence can be removed.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 155 --------------------------------------------------
> 1 file changed, 155 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index df25379..02d3dfd 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -55,10 +55,6 @@ struct imx_chip {
> void __iomem *mmio_base;
>
> struct pwm_chip chip;
> -
> - int (*config)(struct pwm_chip *chip,
> - struct pwm_device *pwm, int duty_ns, int period_ns);
> - void (*set_enable)(struct pwm_chip *chip, bool enable);
> };
>
> #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> @@ -249,145 +245,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> -static int imx_pwm_config_v2(struct pwm_chip *chip,
> - struct pwm_device *pwm, int duty_ns, int period_ns)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> - unsigned long long c;
> - unsigned long period_cycles, duty_cycles, prescale;
> - bool enable = pwm_is_enabled(pwm);
> - u32 cr;
> -
> - /*
> - * i.MX PWMv2 has a 4-word sample FIFO.
> - * In order to avoid FIFO overflow issue, we do software reset
> - * to clear all sample FIFO if the controller is disabled or
> - * wait for a full PWM cycle to get a relinquished FIFO slot
> - * when the controller is enabled and the FIFO is fully loaded.
> - */
> - if (enable)
> - imx_pwm_wait_fifo_slot(chip, pwm);
> - else
> - imx_pwm_sw_reset(chip);
> -
> - c = clk_get_rate(imx->clk_per);
> - c = c * period_ns;
> - do_div(c, 1000000000);
> - period_cycles = c;
> -
> - prescale = period_cycles / 0x10000 + 1;
> -
> - period_cycles /= prescale;
> - c = (unsigned long long)period_cycles * duty_ns;
> - do_div(c, period_ns);
> - duty_cycles = c;
> -
> - /*
> - * according to imx pwm RM, the real period value should be
> - * PERIOD value in PWMPR plus 2.
> - */
> - if (period_cycles > 2)
> - period_cycles -= 2;
> - else
> - period_cycles = 0;
> -
> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> - writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> -
> - cr = MX3_PWMCR_PRESCALER(prescale) |
> - MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> - MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> -
> - if (enable)
> - cr |= MX3_PWMCR_EN;
> -
> - if (pwm->args.polarity == PWM_POLARITY_INVERSED)
> - cr |= MX3_PWMCR_POUTC;
> -
> - writel(cr, imx->mmio_base + MX3_PWMCR);
> -
> - return 0;
> -}
> -
> -static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> - u32 val;
> -
> - val = readl(imx->mmio_base + MX3_PWMCR);
> -
> - if (enable)
> - val |= MX3_PWMCR_EN;
> - else
> - val &= ~MX3_PWMCR_EN;
> -
> - writel(val, imx->mmio_base + MX3_PWMCR);
> -}
> -
> -static int imx_pwm_config(struct pwm_chip *chip,
> - struct pwm_device *pwm, int duty_ns, int period_ns)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> - int ret;
> -
> - ret = clk_prepare_enable(imx->clk_ipg);
> - if (ret)
> - return ret;
> -
> - ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> - clk_disable_unprepare(imx->clk_ipg);
> -
> - return ret;
> -}
> -
> -static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> - int ret;
> -
> - ret = clk_prepare_enable(imx->clk_per);
> - if (ret)
> - return ret;
> -
> - imx->set_enable(chip, true);
> -
> - return 0;
> -}
> -
> -static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> -
> - imx->set_enable(chip, false);
> -
> - clk_disable_unprepare(imx->clk_per);
> -}
> -
> -static int imx_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> - enum pwm_polarity polarity)
> -{
> - struct imx_chip *imx = to_imx_chip(chip);
> - u32 val;
> -
> - if (polarity == pwm->args.polarity)
> - return 0;
> -
> - val = readl(imx->mmio_base + MX3_PWMCR);
> -
> - if (polarity == PWM_POLARITY_INVERSED)
> - val |= MX3_PWMCR_POUTC;
> - else
> - val &= ~MX3_PWMCR_POUTC;
> -
> - writel(val, imx->mmio_base + MX3_PWMCR);
> -
> - dev_dbg(imx->chip.dev, "%s: polarity set to %s\n", __func__,
> - polarity == PWM_POLARITY_INVERSED ? "inverted" : "normal");
> -
> - return 0;
> -}
> -
> static struct pwm_ops imx_pwm_ops_v1 = {
> .enable = imx_pwm_enable_v1,
> .disable = imx_pwm_disable_v1,
> @@ -396,18 +253,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
> };
>
> static struct pwm_ops imx_pwm_ops_v2 = {
> - .enable = imx_pwm_enable,
> - .disable = imx_pwm_disable,
> - .set_polarity = imx_pwm_set_polarity,
> - .config = imx_pwm_config,
> .apply = imx_pwm_apply_v2,
> .owner = THIS_MODULE,
> };
>
> struct imx_pwm_data {
> - int (*config)(struct pwm_chip *chip,
> - struct pwm_device *pwm, int duty_ns, int period_ns);
> - void (*set_enable)(struct pwm_chip *chip, bool enable);
> struct pwm_ops *pwm_ops;
> };
>
> @@ -416,8 +266,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> };
>
> static struct imx_pwm_data imx_pwm_data_v2 = {
> - .config = imx_pwm_config_v2,
> - .set_enable = imx_pwm_set_enable_v2,
> .pwm_ops = &imx_pwm_ops_v2,
> };
>
> @@ -476,9 +324,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> if (IS_ERR(imx->mmio_base))
> return PTR_ERR(imx->mmio_base);
>
> - imx->config = data->config;
> - imx->set_enable = data->set_enable;
> -
> ret = pwmchip_add(&imx->chip);
> if (ret < 0)
> return ret;
On Sun, 23 Oct 2016 23:45:46 +0200
Lukasz Majewski <[email protected]> wrote:
> The need for set_polarity() function has been removed by implementing
> PWM atomic support (apply() callback).
>
> To indicate that the PWMv2 supports polarity inversion, new flag -
> "polarity_supported" has been introduced.
>
> Signed-off-by: Lukasz Majewski <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 02d3dfd..be3034d 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> };
>
> struct imx_pwm_data {
> + bool polarity_supported;
> struct pwm_ops *pwm_ops;
> };
>
> @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> };
>
> static struct imx_pwm_data imx_pwm_data_v2 = {
> + .polarity_supported = true,
> .pwm_ops = &imx_pwm_ops_v2,
> };
>
> @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
> imx->chip.base = -1;
> imx->chip.npwm = 1;
> imx->chip.can_sleep = true;
> - if (data->pwm_ops->set_polarity) {
> + if (data->polarity_supported) {
You're still breaking backward compatibility with DTs defining
#pwm-cells = 2.
Please test the #pwm-cells value before deciding which of_xlate should
be used.
> dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> imx->chip.of_xlate = of_pwm_xlate_with_flags;
> imx->chip.of_pwm_n_cells = 3;
On Mon, 24 Oct 2016 17:28:52 +0200
Boris Brezillon <[email protected]> wrote:
> On Sun, 23 Oct 2016 23:45:46 +0200
> Lukasz Majewski <[email protected]> wrote:
>
> > The need for set_polarity() function has been removed by implementing
> > PWM atomic support (apply() callback).
> >
> > To indicate that the PWMv2 supports polarity inversion, new flag -
> > "polarity_supported" has been introduced.
> >
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > ---
> > drivers/pwm/pwm-imx.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 02d3dfd..be3034d 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > };
> >
> > struct imx_pwm_data {
> > + bool polarity_supported;
> > struct pwm_ops *pwm_ops;
> > };
> >
> > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> > };
> >
> > static struct imx_pwm_data imx_pwm_data_v2 = {
> > + .polarity_supported = true,
> > .pwm_ops = &imx_pwm_ops_v2,
> > };
> >
> > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
> > imx->chip.base = -1;
> > imx->chip.npwm = 1;
> > imx->chip.can_sleep = true;
> > - if (data->pwm_ops->set_polarity) {
> > + if (data->polarity_supported) {
>
> You're still breaking backward compatibility with DTs defining
> #pwm-cells = 2.
>
> Please test the #pwm-cells value before deciding which of_xlate should
> be used.
Nevermind, I didn't look at [1] and [2].
But still, your series is not bisectable: this change should be part of
patch 5 where you remove the ->set_polarity implementation. Otherwise,
this means you don't support polarity setting between patch 5 and 6.
>
> > dev_dbg(&pdev->dev, "PWM supports output inversion\n");
> > imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > imx->chip.of_pwm_n_cells = 3;
>
[1]http://patchwork.ozlabs.org/patch/679706/
[2]http://patchwork.ozlabs.org/patch/679707/
On Sun, 23 Oct 2016 23:45:40 +0200
Lukasz Majewski <[email protected]> wrote:
> This patch set brings atomic operation to i.MX's PWMv2 driver.
>
> This work has been supported and suggested by Boris Brezillon [1] and
> Stefan Agner, by showing how simple the transition could be :-).
>
> It has been divided into several steps:
> - Separate PWMv1 commits from "generic" and non atomic PWM code.
>
> NOTE: Since I do not have board with PWMv1, I would like to ask somebody
> for testing
>
> - Move some imx_config_v2 code to separate functions
>
> - Provide PWM atomic implementation (the ->apply() driver) in a single patch
> for better readability.
>
> - Remove redundant PWM code (disable, enable, config callbacks)
>
> - Clean up the driver infrastructure
>
> - Provide "polarity_supported" flag to indicate support for polarity
> inversion
>
> This work should be applied on top of following commits:
>
> http://patchwork.ozlabs.org/patch/679706/
> http://patchwork.ozlabs.org/patch/679707/
> http://patchwork.ozlabs.org/patch/679680/
I'm not sure I follow the logic here. Has patch [1] already been
applied? If that's not the case, then you should just drop it and put
your changes on top of mainline.
[1]http://patchwork.ozlabs.org/patch/679680/
Hi Boris,
> On Sun, 23 Oct 2016 23:45:42 +0200
> Lukasz Majewski <[email protected]> wrote:
>
> > The software reset code has been extracted from imx_pwm_config_v2
> > function and moved to new one - imx_pwm_sw_reset().
> >
> > This change reduces the overall size of imx_pwm_config_v2() and
> > prepares it for atomic PWM operation.
> >
> > Suggested-by: Stefan Agner <[email protected]>
> > Suggested-by: Boris Brezillon <[email protected]>
> > Signed-off-by: Lukasz Majewski <[email protected]>
>
> Reviewed-by: Boris Brezillon <[email protected]>
>
> Just a nit below ;).
>
> BTW, can't you just merge path 2 and 3?
I do prefer to have many small patches touching and changing just one
thing.
>
> > ---
> > drivers/pwm/pwm-imx.c | 34 ++++++++++++++++++++++------------
> > 1 file changed, 22 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index 83e43d5..fac5c93 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -131,6 +131,25 @@ static void imx_pwm_disable_v1(struct pwm_chip
> > *chip, struct pwm_device *pwm) clk_disable_unprepare(imx->clk_per);
> > }
> >
> > +static void imx_pwm_sw_reset(struct pwm_chip *chip)
> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + struct device *dev = chip->dev;
> > + int wait_count = 0;
> > + u32 cr;
> > +
> > + writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> > + do {
> > + usleep_range(200, 1000);
> > + cr = readl(imx->mmio_base + MX3_PWMCR);
> > + } while ((cr & MX3_PWMCR_SWR) &&
> > + (wait_count++ < MX3_PWM_SWR_LOOP));
> > +
> > + if (cr & MX3_PWMCR_SWR)
> > + dev_warn(dev, "software reset timeout\n");
> > +}
> > +
> > +
> > static int imx_pwm_config_v2(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > @@ -140,7 +159,7 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, unsigned long period_cycles, duty_cycles, prescale;
> > unsigned int period_ms;
> > bool enable = pwm_is_enabled(pwm);
> > - int wait_count = 0, fifoav;
> > + int fifoav;
> > u32 cr, sr;
> >
> > /*
> > @@ -162,17 +181,8 @@ static int imx_pwm_config_v2(struct pwm_chip
> > *chip, if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
> > dev_warn(dev, "there is no free
> > FIFO slot\n"); }
> > - } else {
> > - writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
> > - do {
> > - usleep_range(200, 1000);
> > - cr = readl(imx->mmio_base + MX3_PWMCR);
> > - } while ((cr & MX3_PWMCR_SWR) &&
> > - (wait_count++ < MX3_PWM_SWR_LOOP));
> > -
> > - if (cr & MX3_PWMCR_SWR)
> > - dev_warn(dev, "software reset timeout\n");
> > - }
> > + } else
> > + imx_pwm_sw_reset(chip);
>
> } else {
> imx_pwm_sw_reset(chip);
> }
OK.
>
> >
> > c = clk_get_rate(imx->clk_per);
> > c = c * period_ns;
>
Best regards,
Łukasz Majewski
Hi Boris,
> On Mon, 24 Oct 2016 17:28:52 +0200
> Boris Brezillon <[email protected]> wrote:
>
> > On Sun, 23 Oct 2016 23:45:46 +0200
> > Lukasz Majewski <[email protected]> wrote:
> >
> > > The need for set_polarity() function has been removed by
> > > implementing PWM atomic support (apply() callback).
> > >
> > > To indicate that the PWMv2 supports polarity inversion, new flag -
> > > "polarity_supported" has been introduced.
> > >
> > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > ---
> > > drivers/pwm/pwm-imx.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index 02d3dfd..be3034d 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > > };
> > >
> > > struct imx_pwm_data {
> > > + bool polarity_supported;
> > > struct pwm_ops *pwm_ops;
> > > };
> > >
> > > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> > > };
> > >
> > > static struct imx_pwm_data imx_pwm_data_v2 = {
> > > + .polarity_supported = true,
> > > .pwm_ops = &imx_pwm_ops_v2,
> > > };
> > >
> > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > platform_device *pdev) imx->chip.base = -1;
> > > imx->chip.npwm = 1;
> > > imx->chip.can_sleep = true;
> > > - if (data->pwm_ops->set_polarity) {
> > > + if (data->polarity_supported) {
> >
> > You're still breaking backward compatibility with DTs defining
> > #pwm-cells = 2.
> >
> > Please test the #pwm-cells value before deciding which of_xlate
> > should be used.
>
> Nevermind, I didn't look at [1] and [2].
Yes, some patches are required to make this code work. Especially, I
wanted to explicitly reuse and credit work already done by
Bhuvanchandra.
> But still, your series is not bisectable: this change should be part
> of patch 5 where you remove the ->set_polarity implementation.
> Otherwise, this means you don't support polarity setting between
> patch 5 and 6.
Frankly speaking, I did it on purpose, to have operations in commits
logically separated.
I personally, do detest commits which blur the picture and are not
corresponding to one single logical change - for example remove some
large chunk of code and also add some tiny, new flag.
For me it is not a problem to have polarity disabled between patches 5
and 6, since at the end of the day we have it enabled.
Thanks for your support and review,
Best regards,
Łukasz Majewski
>
> >
> > > dev_dbg(&pdev->dev, "PWM supports output
> > > inversion\n"); imx->chip.of_xlate = of_pwm_xlate_with_flags;
> > > imx->chip.of_pwm_n_cells = 3;
> >
>
> [1]http://patchwork.ozlabs.org/patch/679706/
> [2]http://patchwork.ozlabs.org/patch/679707/
Hi Boris,
> On Sun, 23 Oct 2016 23:45:40 +0200
> Lukasz Majewski <[email protected]> wrote:
>
> > This patch set brings atomic operation to i.MX's PWMv2 driver.
> >
> > This work has been supported and suggested by Boris Brezillon [1]
> > and Stefan Agner, by showing how simple the transition could be :-).
> >
> > It has been divided into several steps:
> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
> >
> > NOTE: Since I do not have board with PWMv1, I would like to ask
> > somebody for testing
> >
> > - Move some imx_config_v2 code to separate functions
> >
> > - Provide PWM atomic implementation (the ->apply() driver) in a
> > single patch for better readability.
> >
> > - Remove redundant PWM code (disable, enable, config callbacks)
> >
> > - Clean up the driver infrastructure
> >
> > - Provide "polarity_supported" flag to indicate support for
> > polarity inversion
> >
> > This work should be applied on top of following commits:
> >
> > http://patchwork.ozlabs.org/patch/679706/
[2]
> > http://patchwork.ozlabs.org/patch/679707/
[3]
> > http://patchwork.ozlabs.org/patch/679680/
>
> I'm not sure I follow the logic here. Has patch [1] already been
> applied? If that's not the case, then you should just drop it and put
> your changes on top of mainline.
>
> [1]http://patchwork.ozlabs.org/patch/679680/
Patches [2] and [3] have been developed initially by Lothar and
subsequently picked up by Bhuvanchandra. There is no issue with them.
The patch [1] is a bit more tricky. The work has been done by
Bhuvanchandra, which adds DTS and core support for polarity inversion.
This code works and utilizes the "old" PWM API with enable, disable and
config. However, Stefan had some comments about the placement for the
polarity setting (in the .config_v2()) and proposed switch to atomic
API.
To make things easier and cleaner, I decided to put my atomic API
rework on top of those patches. In this way I can credit the previous
work and avoid rewriting DTS polarity inversion code already developed
and validated by Bhuvanchandra.
Best regards,
Łukasz Majewski
Hi Lukasz,
Thanks for your work, great to see this coming along! :-)
On 2016-10-24 23:26, Lukasz Majewski wrote:
> Hi Boris,
>
>> On Sun, 23 Oct 2016 23:45:40 +0200
>> Lukasz Majewski <[email protected]> wrote:
>>
>> > This patch set brings atomic operation to i.MX's PWMv2 driver.
>> >
>> > This work has been supported and suggested by Boris Brezillon [1]
>> > and Stefan Agner, by showing how simple the transition could be :-).
>> >
>> > It has been divided into several steps:
>> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
>> >
>> > NOTE: Since I do not have board with PWMv1, I would like to ask
>> > somebody for testing
>> >
>> > - Move some imx_config_v2 code to separate functions
>> >
>> > - Provide PWM atomic implementation (the ->apply() driver) in a
>> > single patch for better readability.
>> >
>> > - Remove redundant PWM code (disable, enable, config callbacks)
>> >
>> > - Clean up the driver infrastructure
>> >
>> > - Provide "polarity_supported" flag to indicate support for
>> > polarity inversion
>> >
>> > This work should be applied on top of following commits:
>> >
>> > http://patchwork.ozlabs.org/patch/679706/
> [2]
>
>> > http://patchwork.ozlabs.org/patch/679707/
> [3]
>
>> > http://patchwork.ozlabs.org/patch/679680/
>>
>> I'm not sure I follow the logic here. Has patch [1] already been
>> applied? If that's not the case, then you should just drop it and put
>> your changes on top of mainline.
>>
>> [1]http://patchwork.ozlabs.org/patch/679680/
>
> Patches [2] and [3] have been developed initially by Lothar and
> subsequently picked up by Bhuvanchandra. There is no issue with them.
As such none of this will get merged since all patchset have known
flaws...
Generally, it is ok to refer to other patchset being a prerequisite, but
that only makes sense if those patch set are still actively worked on
(by somebody other than you).
In this case I really recommend to create a new, complete patchset.
>
> The patch [1] is a bit more tricky. The work has been done by
> Bhuvanchandra, which adds DTS and core support for polarity inversion.
>
> This code works and utilizes the "old" PWM API with enable, disable and
> config. However, Stefan had some comments about the placement for the
> polarity setting (in the .config_v2()) and proposed switch to atomic
> API.
Part of the reason I advocated for the atomic API is to make adding the
polarity functionality easier. It does not archive this goal if we add
the "flawed" code first and then transition to the atomic API.
>
> To make things easier and cleaner, I decided to put my atomic API
> rework on top of those patches. In this way I can credit the previous
> work and avoid rewriting DTS polarity inversion code already developed
> and validated by Bhuvanchandra.
When you apply the patches using git apply, the authorship and signoffs
will stay. There is no problem in including other peoples work into your
patchset, credit will still be given. If you have to change another
persons patch, you typically also add your signoff to show that you
worked on it too.
Here is how I would do it:
1. Start a new branch from mainline (or even -next).
2. Implement the transition to the new atomic API and test it as such
alone (this way we have no polarity support influence yet, just clean
transition to a new API)
3. Cherry pick the PWM core changes for the optional 2/3 args driver
support (they should apply cleanly)
4. Cherry pick (they likely will fail to merge) or reimplement the PWM
polarity driver changes on top of atomic API
5. Cherry pick device tree changes
With this approach we'll end up with a nice history where we should end
up with a fully functional PWM system between every patch.
Btw, past perfect tense is not really usual in commit messages.
SubmittingPatches chapter 2 has some tips on writing good commit
messages:
https://www.kernel.org/doc/Documentation/SubmittingPatches
--
Stefan
On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> The code has been rewritten to remove "generic" calls to
> imx_pwm_{enable|disable|config}.
>
> Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> implementation.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index c37d223..83e43d5 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -66,8 +66,6 @@ struct imx_chip {
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> - struct imx_chip *imx = to_imx_chip(chip);
> -
> /*
> * The PWM subsystem allows for exact frequencies. However,
> * I cannot connect a scope on my device to the PWM line and
> @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> * both the prescaler (/1 .. /128) and then by CLKSEL
> * (/2 .. /16).
> */
> + struct imx_chip *imx = to_imx_chip(chip);
> u32 max = readl(imx->mmio_base + MX1_PWMP);
> u32 p = max * duty_ns / period_ns;
> + int ret;
> +
> + ret = clk_prepare_enable(imx->clk_ipg);
> + if (ret)
> + return ret;
> +
> writel(max - p, imx->mmio_base + MX1_PWMS);
>
> + clk_disable_unprepare(imx->clk_ipg);
> +
> return 0;
> }
>
> -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct imx_chip *imx = to_imx_chip(chip);
> + int ret;
> u32 val;
>
> + ret = clk_prepare_enable(imx->clk_ipg);
> + if (ret)
> + return ret;
> +
> val = readl(imx->mmio_base + MX1_PWMC);
> + val |= MX1_PWMC_EN;
> + writel(val, imx->mmio_base + MX1_PWMC);
>
> - if (enable)
> - val |= MX1_PWMC_EN;
> - else
> - val &= ~MX1_PWMC_EN;
> + clk_disable_unprepare(imx->clk_per);
This looks wrong. You start by enabling clk_ipg which is needed for
register access, but then end with disabling clk_per which is needed for
driving the actual PWM output. This function should probably enable
clk_per on entry and enable clk_ipg while accessing registers.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, 25 Oct 2016 07:54:54 +0200
Sascha Hauer <[email protected]> wrote:
> On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> >
> > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > implementation.
> >
> > Suggested-by: Stefan Agner <[email protected]>
> > Suggested-by: Boris Brezillon <[email protected]>
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > ---
> > drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 34 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index c37d223..83e43d5 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -66,8 +66,6 @@ struct imx_chip {
> > static int imx_pwm_config_v1(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > - struct imx_chip *imx = to_imx_chip(chip);
> > -
> > /*
> > * The PWM subsystem allows for exact frequencies. However,
> > * I cannot connect a scope on my device to the PWM line and
> > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > * both the prescaler (/1 .. /128) and then by CLKSEL
> > * (/2 .. /16).
> > */
> > + struct imx_chip *imx = to_imx_chip(chip);
> > u32 max = readl(imx->mmio_base + MX1_PWMP);
> > u32 p = max * duty_ns / period_ns;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(imx->clk_ipg);
> > + if (ret)
> > + return ret;
> > +
> > writel(max - p, imx->mmio_base + MX1_PWMS);
> >
> > + clk_disable_unprepare(imx->clk_ipg);
> > +
> > return 0;
> > }
> >
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct imx_chip *imx = to_imx_chip(chip);
> > + int ret;
> > u32 val;
> >
> > + ret = clk_prepare_enable(imx->clk_ipg);
> > + if (ret)
> > + return ret;
> > +
> > val = readl(imx->mmio_base + MX1_PWMC);
> > + val |= MX1_PWMC_EN;
> > + writel(val, imx->mmio_base + MX1_PWMC);
> >
> > - if (enable)
> > - val |= MX1_PWMC_EN;
> > - else
> > - val &= ~MX1_PWMC_EN;
> > + clk_disable_unprepare(imx->clk_per);
>
> This looks wrong. You start by enabling clk_ipg which is needed for
> register access, but then end with disabling clk_per which is needed for
> driving the actual PWM output. This function should probably enable
> clk_per on entry and enable clk_ipg while accessing registers.
Oh, I didn't notice there was 2 different clocks here. This probably
means you have to enable clk_ipg when manipulating the registers in
->disable().
One question, if there's a separate clk for the registers, why don't we
leave this clock enabled, and disable it in ->suspend() or ->remove()
instead of enabling/disabling it each time we access the registers?
On Tue, Oct 25, 2016 at 08:27:37AM +0200, Boris Brezillon wrote:
> On Tue, 25 Oct 2016 07:54:54 +0200
> Sascha Hauer <[email protected]> wrote:
>
> > On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > > The code has been rewritten to remove "generic" calls to
> > > imx_pwm_{enable|disable|config}.
> > >
> > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > > implementation.
> > >
> > > Suggested-by: Stefan Agner <[email protected]>
> > > Suggested-by: Boris Brezillon <[email protected]>
> > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > ---
> > > drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> > > 1 file changed, 34 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index c37d223..83e43d5 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -66,8 +66,6 @@ struct imx_chip {
> > > static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > {
> > > - struct imx_chip *imx = to_imx_chip(chip);
> > > -
> > > /*
> > > * The PWM subsystem allows for exact frequencies. However,
> > > * I cannot connect a scope on my device to the PWM line and
> > > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > * both the prescaler (/1 .. /128) and then by CLKSEL
> > > * (/2 .. /16).
> > > */
> > > + struct imx_chip *imx = to_imx_chip(chip);
> > > u32 max = readl(imx->mmio_base + MX1_PWMP);
> > > u32 p = max * duty_ns / period_ns;
> > > + int ret;
> > > +
> > > + ret = clk_prepare_enable(imx->clk_ipg);
> > > + if (ret)
> > > + return ret;
> > > +
> > > writel(max - p, imx->mmio_base + MX1_PWMS);
> > >
> > > + clk_disable_unprepare(imx->clk_ipg);
> > > +
> > > return 0;
> > > }
> > >
> > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > > {
> > > struct imx_chip *imx = to_imx_chip(chip);
> > > + int ret;
> > > u32 val;
> > >
> > > + ret = clk_prepare_enable(imx->clk_ipg);
> > > + if (ret)
> > > + return ret;
> > > +
> > > val = readl(imx->mmio_base + MX1_PWMC);
> > > + val |= MX1_PWMC_EN;
> > > + writel(val, imx->mmio_base + MX1_PWMC);
> > >
> > > - if (enable)
> > > - val |= MX1_PWMC_EN;
> > > - else
> > > - val &= ~MX1_PWMC_EN;
> > > + clk_disable_unprepare(imx->clk_per);
> >
> > This looks wrong. You start by enabling clk_ipg which is needed for
> > register access, but then end with disabling clk_per which is needed for
> > driving the actual PWM output. This function should probably enable
> > clk_per on entry and enable clk_ipg while accessing registers.
>
> Oh, I didn't notice there was 2 different clocks here. This probably
> means you have to enable clk_ipg when manipulating the registers in
> ->disable().
>
> One question, if there's a separate clk for the registers, why don't we
> leave this clock enabled, and disable it in ->suspend() or ->remove()
> instead of enabling/disabling it each time we access the registers?
Well, if we don't need this clock, then why not save the power and
disable it?
However, I'll have to ask Philipp about this clock. It was introduced in
commit 7b27c160c68152581c702b9f1fe362338d2a0cad
Author: Philipp Zabel <[email protected]>
Date: Mon Jun 25 16:15:20 2012 +0200
And even back then the additional clock was not enabled for all register
accesses, so handling this seems broken from the start.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Lukasz,
On Mon, 24 Oct 2016 23:14:58 +0200
Lukasz Majewski <[email protected]> wrote:
> Hi Boris,
>
> > On Mon, 24 Oct 2016 17:28:52 +0200
> > Boris Brezillon <[email protected]> wrote:
> >
> > > On Sun, 23 Oct 2016 23:45:46 +0200
> > > Lukasz Majewski <[email protected]> wrote:
> > >
> > > > The need for set_polarity() function has been removed by
> > > > implementing PWM atomic support (apply() callback).
> > > >
> > > > To indicate that the PWMv2 supports polarity inversion, new flag -
> > > > "polarity_supported" has been introduced.
> > > >
> > > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-imx.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index 02d3dfd..be3034d 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > > > };
> > > >
> > > > struct imx_pwm_data {
> > > > + bool polarity_supported;
> > > > struct pwm_ops *pwm_ops;
> > > > };
> > > >
> > > > @@ -266,6 +267,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
> > > > };
> > > >
> > > > static struct imx_pwm_data imx_pwm_data_v2 = {
> > > > + .polarity_supported = true,
> > > > .pwm_ops = &imx_pwm_ops_v2,
> > > > };
> > > >
> > > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > > platform_device *pdev) imx->chip.base = -1;
> > > > imx->chip.npwm = 1;
> > > > imx->chip.can_sleep = true;
> > > > - if (data->pwm_ops->set_polarity) {
> > > > + if (data->polarity_supported) {
> > >
> > > You're still breaking backward compatibility with DTs defining
> > > #pwm-cells = 2.
> > >
> > > Please test the #pwm-cells value before deciding which of_xlate
> > > should be used.
> >
> > Nevermind, I didn't look at [1] and [2].
>
> Yes, some patches are required to make this code work. Especially, I
> wanted to explicitly reuse and credit work already done by
> Bhuvanchandra.
>
> > But still, your series is not bisectable: this change should be part
> > of patch 5 where you remove the ->set_polarity implementation.
> > Otherwise, this means you don't support polarity setting between
> > patch 5 and 6.
>
> Frankly speaking, I did it on purpose, to have operations in commits
> logically separated.
>
> I personally, do detest commits which blur the picture and are not
> corresponding to one single logical change - for example remove some
> large chunk of code and also add some tiny, new flag.
>
> For me it is not a problem to have polarity disabled between patches 5
> and 6, since at the end of the day we have it enabled.
It's really simple to make this series bisectable, all you have to do
is move patch 6 before patch 5. This being said, I really think you
should follow Stefan's recommendation: base your changes on mainline
and switch to the atomic hook before supporting polarity setting.
Regards,
Boris
On Tue, 25 Oct 2016 08:32:59 +0200
Sascha Hauer <[email protected]> wrote:
> On Tue, Oct 25, 2016 at 08:27:37AM +0200, Boris Brezillon wrote:
> > On Tue, 25 Oct 2016 07:54:54 +0200
> > Sascha Hauer <[email protected]> wrote:
> >
> > > On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > > > The code has been rewritten to remove "generic" calls to
> > > > imx_pwm_{enable|disable|config}.
> > > >
> > > > Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
> > > > implementation.
> > > >
> > > > Suggested-by: Stefan Agner <[email protected]>
> > > > Suggested-by: Boris Brezillon <[email protected]>
> > > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-imx.c | 46 ++++++++++++++++++++++++++++++++++------------
> > > > 1 file changed, 34 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > index c37d223..83e43d5 100644
> > > > --- a/drivers/pwm/pwm-imx.c
> > > > +++ b/drivers/pwm/pwm-imx.c
> > > > @@ -66,8 +66,6 @@ struct imx_chip {
> > > > static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > > struct pwm_device *pwm, int duty_ns, int period_ns)
> > > > {
> > > > - struct imx_chip *imx = to_imx_chip(chip);
> > > > -
> > > > /*
> > > > * The PWM subsystem allows for exact frequencies. However,
> > > > * I cannot connect a scope on my device to the PWM line and
> > > > @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
> > > > * both the prescaler (/1 .. /128) and then by CLKSEL
> > > > * (/2 .. /16).
> > > > */
> > > > + struct imx_chip *imx = to_imx_chip(chip);
> > > > u32 max = readl(imx->mmio_base + MX1_PWMP);
> > > > u32 p = max * duty_ns / period_ns;
> > > > + int ret;
> > > > +
> > > > + ret = clk_prepare_enable(imx->clk_ipg);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > writel(max - p, imx->mmio_base + MX1_PWMS);
> > > >
> > > > + clk_disable_unprepare(imx->clk_ipg);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
> > > > +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
> > > > {
> > > > struct imx_chip *imx = to_imx_chip(chip);
> > > > + int ret;
> > > > u32 val;
> > > >
> > > > + ret = clk_prepare_enable(imx->clk_ipg);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > val = readl(imx->mmio_base + MX1_PWMC);
> > > > + val |= MX1_PWMC_EN;
> > > > + writel(val, imx->mmio_base + MX1_PWMC);
> > > >
> > > > - if (enable)
> > > > - val |= MX1_PWMC_EN;
> > > > - else
> > > > - val &= ~MX1_PWMC_EN;
> > > > + clk_disable_unprepare(imx->clk_per);
> > >
> > > This looks wrong. You start by enabling clk_ipg which is needed for
> > > register access, but then end with disabling clk_per which is needed for
> > > driving the actual PWM output. This function should probably enable
> > > clk_per on entry and enable clk_ipg while accessing registers.
> >
> > Oh, I didn't notice there was 2 different clocks here. This probably
> > means you have to enable clk_ipg when manipulating the registers in
> > ->disable().
> >
> > One question, if there's a separate clk for the registers, why don't we
> > leave this clock enabled, and disable it in ->suspend() or ->remove()
> > instead of enabling/disabling it each time we access the registers?
>
> Well, if we don't need this clock, then why not save the power and
> disable it?
That's true, especially if enabling the clock is instantaneous (i.e. the
clock driver does not have to wait for the clk to be ready).
> However, I'll have to ask Philipp about this clock. It was introduced in
>
> commit 7b27c160c68152581c702b9f1fe362338d2a0cad
> Author: Philipp Zabel <[email protected]>
> Date: Mon Jun 25 16:15:20 2012 +0200
>
> And even back then the additional clock was not enabled for all register
> accesses, so handling this seems broken from the start.
>
> Sascha
>
>
Hi Sascha,
> On Sun, Oct 23, 2016 at 11:45:41PM +0200, Lukasz Majewski wrote:
> > The code has been rewritten to remove "generic" calls to
> > imx_pwm_{enable|disable|config}.
> >
> > Such approach would facilitate switch to atomic PWM (a.k.a
> > ->apply()) implementation.
> >
> > Suggested-by: Stefan Agner <[email protected]>
> > Suggested-by: Boris Brezillon <[email protected]>
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > ---
> > drivers/pwm/pwm-imx.c | 46
> > ++++++++++++++++++++++++++++++++++------------ 1 file changed, 34
> > insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index c37d223..83e43d5 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -66,8 +66,6 @@ struct imx_chip {
> > static int imx_pwm_config_v1(struct pwm_chip *chip,
> > struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > - struct imx_chip *imx = to_imx_chip(chip);
> > -
> > /*
> > * The PWM subsystem allows for exact frequencies. However,
> > * I cannot connect a scope on my device to the PWM line
> > and @@ -85,26 +83,52 @@ static int imx_pwm_config_v1(struct
> > pwm_chip *chip,
> > * both the prescaler (/1 .. /128) and then by CLKSEL
> > * (/2 .. /16).
> > */
> > + struct imx_chip *imx = to_imx_chip(chip);
> > u32 max = readl(imx->mmio_base + MX1_PWMP);
> > u32 p = max * duty_ns / period_ns;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(imx->clk_ipg);
> > + if (ret)
> > + return ret;
> > +
> > writel(max - p, imx->mmio_base + MX1_PWMS);
> >
> > + clk_disable_unprepare(imx->clk_ipg);
> > +
> > return 0;
> > }
> >
> > -static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool
> > enable) +static int imx_pwm_enable_v1(struct pwm_chip *chip, struct
> > pwm_device *pwm) {
> > struct imx_chip *imx = to_imx_chip(chip);
> > + int ret;
> > u32 val;
> >
> > + ret = clk_prepare_enable(imx->clk_ipg);
> > + if (ret)
> > + return ret;
> > +
> > val = readl(imx->mmio_base + MX1_PWMC);
> > + val |= MX1_PWMC_EN;
> > + writel(val, imx->mmio_base + MX1_PWMC);
> >
> > - if (enable)
> > - val |= MX1_PWMC_EN;
> > - else
> > - val &= ~MX1_PWMC_EN;
> > + clk_disable_unprepare(imx->clk_per);
>
> This looks wrong. You start by enabling clk_ipg which is needed for
> register access, but then end with disabling clk_per which is needed
> for driving the actual PWM output. This function should probably
> enable clk_per on entry and enable clk_ipg while accessing registers.
This was my omission. You are right, there are two clocks. One is
feeding PWM block and another allows access to registers.
Similar scheme is used with PWMv2. I will correct this when preparing
version 2 of this patch set.
Thanks for feedback,
Łukasz Majewski
>
> Sascha
>
Hi Boris,
> Hi Lukasz,
>
> On Mon, 24 Oct 2016 23:14:58 +0200
> Lukasz Majewski <[email protected]> wrote:
>
> > Hi Boris,
> >
> > > On Mon, 24 Oct 2016 17:28:52 +0200
> > > Boris Brezillon <[email protected]> wrote:
> > >
> > > > On Sun, 23 Oct 2016 23:45:46 +0200
> > > > Lukasz Majewski <[email protected]> wrote:
> > > >
> > > > > The need for set_polarity() function has been removed by
> > > > > implementing PWM atomic support (apply() callback).
> > > > >
> > > > > To indicate that the PWMv2 supports polarity inversion, new
> > > > > flag - "polarity_supported" has been introduced.
> > > > >
> > > > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > > > ---
> > > > > drivers/pwm/pwm-imx.c | 4 +++-
> > > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > > > index 02d3dfd..be3034d 100644
> > > > > --- a/drivers/pwm/pwm-imx.c
> > > > > +++ b/drivers/pwm/pwm-imx.c
> > > > > @@ -258,6 +258,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
> > > > > };
> > > > >
> > > > > struct imx_pwm_data {
> > > > > + bool polarity_supported;
> > > > > struct pwm_ops *pwm_ops;
> > > > > };
> > > > >
> > > > > @@ -266,6 +267,7 @@ static struct imx_pwm_data
> > > > > imx_pwm_data_v1 = { };
> > > > >
> > > > > static struct imx_pwm_data imx_pwm_data_v2 = {
> > > > > + .polarity_supported = true,
> > > > > .pwm_ops = &imx_pwm_ops_v2,
> > > > > };
> > > > >
> > > > > @@ -313,7 +315,7 @@ static int imx_pwm_probe(struct
> > > > > platform_device *pdev) imx->chip.base = -1;
> > > > > imx->chip.npwm = 1;
> > > > > imx->chip.can_sleep = true;
> > > > > - if (data->pwm_ops->set_polarity) {
> > > > > + if (data->polarity_supported) {
> > > >
> > > > You're still breaking backward compatibility with DTs defining
> > > > #pwm-cells = 2.
> > > >
> > > > Please test the #pwm-cells value before deciding which of_xlate
> > > > should be used.
> > >
> > > Nevermind, I didn't look at [1] and [2].
> >
> > Yes, some patches are required to make this code work. Especially, I
> > wanted to explicitly reuse and credit work already done by
> > Bhuvanchandra.
> >
> > > But still, your series is not bisectable: this change should be
> > > part of patch 5 where you remove the ->set_polarity
> > > implementation. Otherwise, this means you don't support polarity
> > > setting between patch 5 and 6.
> >
> > Frankly speaking, I did it on purpose, to have operations in commits
> > logically separated.
> >
> > I personally, do detest commits which blur the picture and are not
> > corresponding to one single logical change - for example remove some
> > large chunk of code and also add some tiny, new flag.
> >
> > For me it is not a problem to have polarity disabled between
> > patches 5 and 6, since at the end of the day we have it enabled.
>
> It's really simple to make this series bisectable, all you have to do
> is move patch 6 before patch 5.
Hmm... You are right, I do wonder why I didn't get this idea from the
very beginning.
> This being said, I really think you
> should follow Stefan's recommendation: base your changes on mainline
> and switch to the atomic hook before supporting polarity setting.
I will do my best :-)
Best regards,
Łukasz Majewski
>
> Regards,
>
> Boris
Hi Stefan,
> Hi Lukasz,
>
> Thanks for your work, great to see this coming along! :-)
>
> On 2016-10-24 23:26, Lukasz Majewski wrote:
> > Hi Boris,
> >
> >> On Sun, 23 Oct 2016 23:45:40 +0200
> >> Lukasz Majewski <[email protected]> wrote:
> >>
> >> > This patch set brings atomic operation to i.MX's PWMv2 driver.
> >> >
> >> > This work has been supported and suggested by Boris Brezillon [1]
> >> > and Stefan Agner, by showing how simple the transition could
> >> > be :-).
> >> >
> >> > It has been divided into several steps:
> >> > - Separate PWMv1 commits from "generic" and non atomic PWM code.
> >> >
> >> > NOTE: Since I do not have board with PWMv1, I would like to ask
> >> > somebody for testing
> >> >
> >> > - Move some imx_config_v2 code to separate functions
> >> >
> >> > - Provide PWM atomic implementation (the ->apply() driver) in a
> >> > single patch for better readability.
> >> >
> >> > - Remove redundant PWM code (disable, enable, config callbacks)
> >> >
> >> > - Clean up the driver infrastructure
> >> >
> >> > - Provide "polarity_supported" flag to indicate support for
> >> > polarity inversion
> >> >
> >> > This work should be applied on top of following commits:
> >> >
> >> > http://patchwork.ozlabs.org/patch/679706/
> > [2]
> >
> >> > http://patchwork.ozlabs.org/patch/679707/
> > [3]
> >
> >> > http://patchwork.ozlabs.org/patch/679680/
> >>
> >> I'm not sure I follow the logic here. Has patch [1] already been
> >> applied? If that's not the case, then you should just drop it and
> >> put your changes on top of mainline.
> >>
> >> [1]http://patchwork.ozlabs.org/patch/679680/
> >
> > Patches [2] and [3] have been developed initially by Lothar and
> > subsequently picked up by Bhuvanchandra. There is no issue with
> > them.
>
> As such none of this will get merged since all patchset have known
> flaws...
>
> Generally, it is ok to refer to other patchset being a prerequisite,
> but that only makes sense if those patch set are still actively
> worked on (by somebody other than you).
>
> In this case I really recommend to create a new, complete patchset.
>
> >
> > The patch [1] is a bit more tricky. The work has been done by
> > Bhuvanchandra, which adds DTS and core support for polarity
> > inversion.
> >
> > This code works and utilizes the "old" PWM API with enable, disable
> > and config. However, Stefan had some comments about the placement
> > for the polarity setting (in the .config_v2()) and proposed switch
> > to atomic API.
>
> Part of the reason I advocated for the atomic API is to make adding
> the polarity functionality easier. It does not archive this goal if
> we add the "flawed" code first and then transition to the atomic API.
>
> >
> > To make things easier and cleaner, I decided to put my atomic API
> > rework on top of those patches. In this way I can credit the
> > previous work and avoid rewriting DTS polarity inversion code
> > already developed and validated by Bhuvanchandra.
>
> When you apply the patches using git apply, the authorship and
> signoffs will stay. There is no problem in including other peoples
> work into your patchset, credit will still be given. If you have to
> change another persons patch, you typically also add your signoff to
> show that you worked on it too.
I do wanted to reuse as much work as possible (especially that the code
was working).
>
> Here is how I would do it:
>
> 1. Start a new branch from mainline (or even -next).
With the newest mainline 4.9-rcX
SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6
the i.MX6q is not booting. Apparently I do need to wait for things to
calm down.
The last working version is v4.8, which PWM's code is the same as v4.7.
> 2. Implement the transition to the new atomic API and test it as such
> alone (this way we have no polarity support influence yet, just clean
> transition to a new API)
I _just_ needed to add polarity support (by setting one bit) to the
driver, so I ended up with rewriting the whole PWM i.MX driver :-).
> 3. Cherry pick the PWM core changes for the optional 2/3 args driver
> support (they should apply cleanly)
> 4. Cherry pick (they likely will fail to merge) or reimplement the PWM
> polarity driver changes on top of atomic API
> 5. Cherry pick device tree changes
>
> With this approach we'll end up with a nice history where we should
> end up with a fully functional PWM system between every patch.
I'm fine with proposed approach. I will prepare v2 of patches soon.
>
> Btw, past perfect tense is not really usual in commit messages.
> SubmittingPatches chapter 2 has some tips on writing good commit
> messages:
> https://www.kernel.org/doc/Documentation/SubmittingPatches
OK.
Thanks you for your support,
Best regards,
Łukasz Majewski
>
> --
> Stefan
Hi Lukasz,
On Tue, Oct 25, 2016 at 5:07 AM, Lukasz Majewski <[email protected]> wrote:
> With the newest mainline 4.9-rcX
> SHA:0c2b6dc4fd4fa13796b319aae969a009f03222c6
>
> the i.MX6q is not booting. Apparently I do need to wait for things to
> calm down.
Does this commit help with the boot issue you observed?
On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <[email protected]> wrote:
>> the i.MX6q is not booting. Apparently I do need to wait for things to
>> calm down.
>
> Does this commit help with the boot issue you observed?
Sorry, missed to put the commit. Here it goes:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804
On Tue, 25 Oct 2016 15:09:03 -0200
Fabio Estevam <[email protected]> wrote:
> On Tue, Oct 25, 2016 at 3:08 PM, Fabio Estevam <[email protected]>
> wrote:
>
> >> the i.MX6q is not booting. Apparently I do need to wait for things
> >> to calm down.
> >
> > Does this commit help with the boot issue you observed?
>
> Sorry, missed to put the commit. Here it goes:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/arch/arm/mach-imx/gpc.c?id=eef0b282bb586259d35548851cf6a4ce847bb804
I can confirm that the above patch fixed the problem.
My iMX6q board boots on 4.9.0-rc2
Thanks for support,
Łukasz Majewski
From: Sascha Hauer <[email protected]>
The use of the ipg clock was introduced with commit 7b27c160c681
("pwm: i.MX: fix clock lookup").
In the commit message it was claimed that the ipg clock is enabled for
register accesses. This is true for the ->config() callback, but not
for the ->set_enable() callback. Given that the ipg clock is not
consistently enabled for all register accesses we can assume that either
it is not required at all or that the current code does not work.
Remove the ipg clock code for now so that it's no longer in the way of
refactoring the driver.
Signed-off-by: Sascha Hauer <[email protected]>
Cc: Philipp Zabel <[email protected]>
---
[commit message text refactored by Lukasz Majewski <[email protected]>]
---
Changes for v3:
- New patch
---
drivers/pwm/pwm-imx.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d600fd5..70609ef2 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -49,7 +49,6 @@
struct imx_chip {
struct clk *clk_per;
- struct clk *clk_ipg;
void __iomem *mmio_base;
@@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
struct imx_chip *imx = to_imx_chip(chip);
- int ret;
-
- ret = clk_prepare_enable(imx->clk_ipg);
- if (ret)
- return ret;
- ret = imx->config(chip, pwm, duty_ns, period_ns);
-
- clk_disable_unprepare(imx->clk_ipg);
-
- return ret;
+ return imx->config(chip, pwm, duty_ns, period_ns);
}
static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
return PTR_ERR(imx->clk_per);
}
- imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
- if (IS_ERR(imx->clk_ipg)) {
- dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
- PTR_ERR(imx->clk_ipg));
- return PTR_ERR(imx->clk_ipg);
- }
-
imx->chip.ops = &imx_pwm_ops;
imx->chip.dev = &pdev->dev;
imx->chip.base = -1;
--
2.1.4
This patch provides separate set of pwm ops utilized by
i.MX's PWMv1 and PWMv2.
Signed-off-by: Lothar Waßmann <[email protected]>
Signed-off-by: Bhuvanchandra DV <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
Acked-by: Shawn Guo <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
---
Changes for v3:
- Adjust the code to work with ipg clock removed
Changes for v2:
- New patch
---
drivers/pwm/pwm-imx.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 70609ef2..d594501 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -230,7 +230,14 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(imx->clk_per);
}
-static struct pwm_ops imx_pwm_ops = {
+static struct pwm_ops imx_pwm_ops_v1 = {
+ .enable = imx_pwm_enable,
+ .disable = imx_pwm_disable,
+ .config = imx_pwm_config,
+ .owner = THIS_MODULE,
+};
+
+static struct pwm_ops imx_pwm_ops_v2 = {
.enable = imx_pwm_enable,
.disable = imx_pwm_disable,
.config = imx_pwm_config,
@@ -241,16 +248,19 @@ struct imx_pwm_data {
int (*config)(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns);
void (*set_enable)(struct pwm_chip *chip, bool enable);
+ struct pwm_ops *pwm_ops;
};
static struct imx_pwm_data imx_pwm_data_v1 = {
.config = imx_pwm_config_v1,
.set_enable = imx_pwm_set_enable_v1,
+ .pwm_ops = &imx_pwm_ops_v1,
};
static struct imx_pwm_data imx_pwm_data_v2 = {
.config = imx_pwm_config_v2,
.set_enable = imx_pwm_set_enable_v2,
+ .pwm_ops = &imx_pwm_ops_v2,
};
static const struct of_device_id imx_pwm_dt_ids[] = {
@@ -272,6 +282,8 @@ static int imx_pwm_probe(struct platform_device *pdev)
if (!of_id)
return -ENODEV;
+ data = of_id->data;
+
imx = devm_kzalloc(&pdev->dev, sizeof(*imx), GFP_KERNEL);
if (imx == NULL)
return -ENOMEM;
@@ -283,7 +295,7 @@ static int imx_pwm_probe(struct platform_device *pdev)
return PTR_ERR(imx->clk_per);
}
- imx->chip.ops = &imx_pwm_ops;
+ imx->chip.ops = data->pwm_ops;
imx->chip.dev = &pdev->dev;
imx->chip.base = -1;
imx->chip.npwm = 1;
@@ -294,7 +306,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
if (IS_ERR(imx->mmio_base))
return PTR_ERR(imx->mmio_base);
- data = of_id->data;
imx->config = data->config;
imx->set_enable = data->set_enable;
--
2.1.4
The code has been rewritten to remove "generic" calls to
imx_pwm_{enable|disable|config}.
Such approach would facilitate switch to atomic PWM (a.k.a ->apply())
implementation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Reviewed-by: Stefan Agner <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
[Minor code adjustment according to Stefan Agner comment]
Changes for v3:
- Remove ipg clock
Changes for v2:
- Add missing clock unprepare for clk_ipg
- Enable peripheral PWM clock (clk_per)
---
drivers/pwm/pwm-imx.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index d594501..8f911b9 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -90,19 +90,34 @@ static int imx_pwm_config_v1(struct pwm_chip *chip,
return 0;
}
-static void imx_pwm_set_enable_v1(struct pwm_chip *chip, bool enable)
+static int imx_pwm_enable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct imx_chip *imx = to_imx_chip(chip);
+ int ret;
u32 val;
+ ret = clk_prepare_enable(imx->clk_per);
+ if (ret)
+ return ret;
+
val = readl(imx->mmio_base + MX1_PWMC);
+ val |= MX1_PWMC_EN;
+ writel(val, imx->mmio_base + MX1_PWMC);
- if (enable)
- val |= MX1_PWMC_EN;
- else
- val &= ~MX1_PWMC_EN;
+ return 0;
+}
+
+static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ u32 val;
+
+ val = readl(imx->mmio_base + MX1_PWMC);
+ val &= ~MX1_PWMC_EN;
writel(val, imx->mmio_base + MX1_PWMC);
+
+ clk_disable_unprepare(imx->clk_per);
}
static int imx_pwm_config_v2(struct pwm_chip *chip,
@@ -231,9 +246,9 @@ static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
}
static struct pwm_ops imx_pwm_ops_v1 = {
- .enable = imx_pwm_enable,
- .disable = imx_pwm_disable,
- .config = imx_pwm_config,
+ .enable = imx_pwm_enable_v1,
+ .disable = imx_pwm_disable_v1,
+ .config = imx_pwm_config_v1,
.owner = THIS_MODULE,
};
@@ -252,8 +267,6 @@ struct imx_pwm_data {
};
static struct imx_pwm_data imx_pwm_data_v1 = {
- .config = imx_pwm_config_v1,
- .set_enable = imx_pwm_set_enable_v1,
.pwm_ops = &imx_pwm_ops_v1,
};
--
2.1.4
The code, which waits for fifo slot, has been extracted from
imx_pwm_config_v2 function and moved to new one - imx_pwm_wait_fifo_slot().
This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Reviewed-by: Stefan Agner <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- None
---
drivers/pwm/pwm-imx.c | 43 +++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index b4e5803..ebe9b0c 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -137,18 +137,36 @@ static void imx_pwm_sw_reset(struct pwm_chip *chip)
dev_warn(dev, "software reset timeout\n");
}
+static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
+ struct pwm_device *pwm)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct device *dev = chip->dev;
+ unsigned int period_ms;
+ int fifoav;
+ u32 sr;
+
+ sr = readl(imx->mmio_base + MX3_PWMSR);
+ fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+ if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
+ period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
+ NSEC_PER_MSEC);
+ msleep(period_ms);
+
+ sr = readl(imx->mmio_base + MX3_PWMSR);
+ if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
+ dev_warn(dev, "there is no free FIFO slot\n");
+ }
+}
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
struct imx_chip *imx = to_imx_chip(chip);
- struct device *dev = chip->dev;
unsigned long long c;
unsigned long period_cycles, duty_cycles, prescale;
- unsigned int period_ms;
bool enable = pwm_is_enabled(pwm);
- int fifoav;
- u32 cr, sr;
+ u32 cr;
/*
* i.MX PWMv2 has a 4-word sample FIFO.
@@ -157,21 +175,10 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
* wait for a full PWM cycle to get a relinquished FIFO slot
* when the controller is enabled and the FIFO is fully loaded.
*/
- if (enable) {
- sr = readl(imx->mmio_base + MX3_PWMSR);
- fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
- if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
- period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
- NSEC_PER_MSEC);
- msleep(period_ms);
-
- sr = readl(imx->mmio_base + MX3_PWMSR);
- if (fifoav == (sr & MX3_PWMSR_FIFOAV_MASK))
- dev_warn(dev, "there is no free FIFO slot\n");
- }
- } else {
+ if (enable)
+ imx_pwm_wait_fifo_slot(chip, pwm);
+ else
imx_pwm_sw_reset(chip);
- }
c = clk_get_rate(imx->clk_per);
c = c * period_ns;
--
2.1.4
This commit provides apply() callback implementation for i.MX's PWMv2.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
---
Changes for v3:
- Remove ipg clock enable/disable functions
Changes for v2:
- None
---
drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index ebe9b0c..cd53c05 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
}
}
+static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ unsigned long period_cycles, duty_cycles, prescale;
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct pwm_state cstate;
+ unsigned long long c;
+ u32 cr = 0;
+ int ret;
+
+ pwm_get_state(pwm, &cstate);
+
+ c = clk_get_rate(imx->clk_per);
+ c *= state->period;
+
+ do_div(c, 1000000000);
+ period_cycles = c;
+
+ prescale = period_cycles / 0x10000 + 1;
+
+ period_cycles /= prescale;
+ c = (unsigned long long)period_cycles * state->duty_cycle;
+ do_div(c, state->period);
+ duty_cycles = c;
+
+ /*
+ * according to imx pwm RM, the real period value should be
+ * PERIOD value in PWMPR plus 2.
+ */
+ if (period_cycles > 2)
+ period_cycles -= 2;
+ else
+ period_cycles = 0;
+
+ /* Enable the clock if the PWM is being enabled. */
+ if (state->enabled && !cstate.enabled) {
+ ret = clk_prepare_enable(imx->clk_per);
+ if (ret)
+ return ret;
+ }
+
+ /*
+ * Wait for a free FIFO slot if the PWM is already enabled, and flush
+ * the FIFO if the PWM was disabled and is about to be enabled.
+ */
+ if (cstate.enabled)
+ imx_pwm_wait_fifo_slot(chip, pwm);
+ else if (state->enabled)
+ imx_pwm_sw_reset(chip);
+
+ writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+ writel(period_cycles, imx->mmio_base + MX3_PWMPR);
+
+ cr |= MX3_PWMCR_PRESCALER(prescale) |
+ MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
+ MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
+
+ if (state->enabled)
+ cr |= MX3_PWMCR_EN;
+
+ writel(cr, imx->mmio_base + MX3_PWMCR);
+
+ /* Disable the clock if the PWM is being disabled. */
+ if (!state->enabled && cstate.enabled)
+ clk_disable_unprepare(imx->clk_per);
+
+ return 0;
+}
+
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -273,6 +342,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
.enable = imx_pwm_enable,
.disable = imx_pwm_disable,
.config = imx_pwm_config,
+ .apply = imx_pwm_apply_v2,
.owner = THIS_MODULE,
};
--
2.1.4
From: Lothar Wassmann <[email protected]>
Change the pwm chip driver registration, so that a chip driver that
supports polarity inversion can still be used with DTBs that don't
provide the 'PWM_POLARITY' flag.
This is done to provide polarity inversion support for the pwm-imx
driver without having to modify all existing DTS files.
Signed-off-by: Lothar Wassmann <[email protected]>
Signed-off-by: Bhuvanchandra DV <[email protected]>
Suggested-by: Sascha Hauer <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- None
---
drivers/pwm/core.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ec7179f..d80e5c5 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -137,9 +137,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
{
struct pwm_device *pwm;
+ /* check, whether the driver supports a third cell for flags */
if (pc->of_pwm_n_cells < 3)
return ERR_PTR(-EINVAL);
+ /* flags in the third cell are optional */
+ if (args->args_count < 2)
+ return ERR_PTR(-EINVAL);
+
if (args->args[0] >= pc->npwm)
return ERR_PTR(-EINVAL);
@@ -148,11 +153,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
return pwm;
pwm->args.period = args->args[1];
+ pwm->args.polarity = PWM_POLARITY_NORMAL;
- if (args->args[2] & PWM_POLARITY_INVERTED)
+ if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
pwm->args.polarity = PWM_POLARITY_INVERSED;
- else
- pwm->args.polarity = PWM_POLARITY_NORMAL;
return pwm;
}
@@ -163,9 +167,14 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
{
struct pwm_device *pwm;
+ /* sanity check driver support */
if (pc->of_pwm_n_cells < 2)
return ERR_PTR(-EINVAL);
+ /* all cells are required */
+ if (args->args_count != pc->of_pwm_n_cells)
+ return ERR_PTR(-EINVAL);
+
if (args->args[0] >= pc->npwm)
return ERR_PTR(-EINVAL);
@@ -674,13 +683,6 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
goto put;
}
- if (args.args_count != pc->of_pwm_n_cells) {
- pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name,
- args.np->full_name);
- pwm = ERR_PTR(-EINVAL);
- goto put;
- }
-
pwm = pc->of_xlate(pc, &args);
if (IS_ERR(pwm))
goto put;
--
2.1.4
The imx-pwm.txt documentation update as a preparation for polarity
support.
Signed-off-by: Bhuvanchandra DV <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- New patch
---
Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/pwm/imx-pwm.txt b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
index e00c2e9..c61bdf8 100644
--- a/Documentation/devicetree/bindings/pwm/imx-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/imx-pwm.txt
@@ -6,8 +6,8 @@ Required properties:
- "fsl,imx1-pwm" for PWM compatible with the one integrated on i.MX1
- "fsl,imx27-pwm" for PWM compatible with the one integrated on i.MX27
- reg: physical base address and length of the controller's registers
-- #pwm-cells: should be 2. See pwm.txt in this directory for a description of
- the cells format.
+- #pwm-cells: 2 for i.MX1 and 3 for i.MX27 and newer SoCs. See pwm.txt
+ in this directory for a description of the cells format.
- clocks : Clock specifiers for both ipg and per clocks.
- clock-names : Clock names should include both "ipg" and "per"
See the clock consumer binding,
@@ -17,7 +17,7 @@ See the clock consumer binding,
Example:
pwm1: pwm@53fb4000 {
- #pwm-cells = <2>;
+ #pwm-cells = <3>;
compatible = "fsl,imx53-pwm", "fsl,imx27-pwm";
reg = <0x53fb4000 0x4000>;
clocks = <&clks IMX5_CLK_PWM1_IPG_GATE>,
--
2.1.4
From: Lothar Wassmann <[email protected]>
Make the messages that are printed in case of fatal errors actually
visible to the user without having to recompile the driver with
debugging enabled.
Signed-off-by: Lothar Waßmann <[email protected]>
Signed-off-by: Bhuvanchandra DV <[email protected]>
---
Changes for v3:
- None
---
drivers/pwm/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 172ef82..ec7179f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -663,13 +663,13 @@ struct pwm_device *of_pwm_get(struct device_node *np, const char *con_id)
err = of_parse_phandle_with_args(np, "pwms", "#pwm-cells", index,
&args);
if (err) {
- pr_debug("%s(): can't parse \"pwms\" property\n", __func__);
+ pr_err("%s(): can't parse \"pwms\" property\n", __func__);
return ERR_PTR(err);
}
pc = of_node_to_pwmchip(args.np);
if (IS_ERR(pc)) {
- pr_debug("%s(): PWM chip not found\n", __func__);
+ pr_err("%s(): PWM chip not found\n", __func__);
pwm = ERR_CAST(pc);
goto put;
}
--
2.1.4
The software reset code has been extracted from imx_pwm_config_v2 function
and moved to new one - imx_pwm_sw_reset().
This change reduces the overall size of imx_pwm_config_v2() and prepares
it for atomic PWM operation.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Reviewed-by: Stefan Agner <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- Add missing parenthesis
---
drivers/pwm/pwm-imx.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 8497902..b4e5803 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -119,6 +119,25 @@ static void imx_pwm_disable_v1(struct pwm_chip *chip, struct pwm_device *pwm)
clk_disable_unprepare(imx->clk_per);
}
+static void imx_pwm_sw_reset(struct pwm_chip *chip)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ struct device *dev = chip->dev;
+ int wait_count = 0;
+ u32 cr;
+
+ writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
+ do {
+ usleep_range(200, 1000);
+ cr = readl(imx->mmio_base + MX3_PWMCR);
+ } while ((cr & MX3_PWMCR_SWR) &&
+ (wait_count++ < MX3_PWM_SWR_LOOP));
+
+ if (cr & MX3_PWMCR_SWR)
+ dev_warn(dev, "software reset timeout\n");
+}
+
+
static int imx_pwm_config_v2(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -128,7 +147,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
unsigned long period_cycles, duty_cycles, prescale;
unsigned int period_ms;
bool enable = pwm_is_enabled(pwm);
- int wait_count = 0, fifoav;
+ int fifoav;
u32 cr, sr;
/*
@@ -151,15 +170,7 @@ static int imx_pwm_config_v2(struct pwm_chip *chip,
dev_warn(dev, "there is no free FIFO slot\n");
}
} else {
- writel(MX3_PWMCR_SWR, imx->mmio_base + MX3_PWMCR);
- do {
- usleep_range(200, 1000);
- cr = readl(imx->mmio_base + MX3_PWMCR);
- } while ((cr & MX3_PWMCR_SWR) &&
- (wait_count++ < MX3_PWM_SWR_LOOP));
-
- if (cr & MX3_PWMCR_SWR)
- dev_warn(dev, "software reset timeout\n");
+ imx_pwm_sw_reset(chip);
}
c = clk_get_rate(imx->clk_per);
--
2.1.4
The code providing functionality surpassed by the atomic PWM is not needed
anymore and hence can be removed.
Suggested-by: Stefan Agner <[email protected]>
Suggested-by: Boris Brezillon <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- None
---
drivers/pwm/pwm-imx.c | 118 --------------------------------------------------
1 file changed, 118 deletions(-)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index cd53c05..b636526 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -53,10 +53,6 @@ struct imx_chip {
void __iomem *mmio_base;
struct pwm_chip chip;
-
- int (*config)(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns);
- void (*set_enable)(struct pwm_chip *chip, bool enable);
};
#define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
@@ -228,109 +224,6 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
-static int imx_pwm_config_v2(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- unsigned long long c;
- unsigned long period_cycles, duty_cycles, prescale;
- bool enable = pwm_is_enabled(pwm);
- u32 cr;
-
- /*
- * i.MX PWMv2 has a 4-word sample FIFO.
- * In order to avoid FIFO overflow issue, we do software reset
- * to clear all sample FIFO if the controller is disabled or
- * wait for a full PWM cycle to get a relinquished FIFO slot
- * when the controller is enabled and the FIFO is fully loaded.
- */
- if (enable)
- imx_pwm_wait_fifo_slot(chip, pwm);
- else
- imx_pwm_sw_reset(chip);
-
- c = clk_get_rate(imx->clk_per);
- c = c * period_ns;
- do_div(c, 1000000000);
- period_cycles = c;
-
- prescale = period_cycles / 0x10000 + 1;
-
- period_cycles /= prescale;
- c = (unsigned long long)period_cycles * duty_ns;
- do_div(c, period_ns);
- duty_cycles = c;
-
- /*
- * according to imx pwm RM, the real period value should be
- * PERIOD value in PWMPR plus 2.
- */
- if (period_cycles > 2)
- period_cycles -= 2;
- else
- period_cycles = 0;
-
- writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
- writel(period_cycles, imx->mmio_base + MX3_PWMPR);
-
- cr = MX3_PWMCR_PRESCALER(prescale) |
- MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
- MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
-
- if (enable)
- cr |= MX3_PWMCR_EN;
-
- writel(cr, imx->mmio_base + MX3_PWMCR);
-
- return 0;
-}
-
-static void imx_pwm_set_enable_v2(struct pwm_chip *chip, bool enable)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- u32 val;
-
- val = readl(imx->mmio_base + MX3_PWMCR);
-
- if (enable)
- val |= MX3_PWMCR_EN;
- else
- val &= ~MX3_PWMCR_EN;
-
- writel(val, imx->mmio_base + MX3_PWMCR);
-}
-
-static int imx_pwm_config(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns)
-{
- struct imx_chip *imx = to_imx_chip(chip);
-
- return imx->config(chip, pwm, duty_ns, period_ns);
-}
-
-static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct imx_chip *imx = to_imx_chip(chip);
- int ret;
-
- ret = clk_prepare_enable(imx->clk_per);
- if (ret)
- return ret;
-
- imx->set_enable(chip, true);
-
- return 0;
-}
-
-static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct imx_chip *imx = to_imx_chip(chip);
-
- imx->set_enable(chip, false);
-
- clk_disable_unprepare(imx->clk_per);
-}
-
static struct pwm_ops imx_pwm_ops_v1 = {
.enable = imx_pwm_enable_v1,
.disable = imx_pwm_disable_v1,
@@ -339,17 +232,11 @@ static struct pwm_ops imx_pwm_ops_v1 = {
};
static struct pwm_ops imx_pwm_ops_v2 = {
- .enable = imx_pwm_enable,
- .disable = imx_pwm_disable,
- .config = imx_pwm_config,
.apply = imx_pwm_apply_v2,
.owner = THIS_MODULE,
};
struct imx_pwm_data {
- int (*config)(struct pwm_chip *chip,
- struct pwm_device *pwm, int duty_ns, int period_ns);
- void (*set_enable)(struct pwm_chip *chip, bool enable);
struct pwm_ops *pwm_ops;
};
@@ -358,8 +245,6 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
};
static struct imx_pwm_data imx_pwm_data_v2 = {
- .config = imx_pwm_config_v2,
- .set_enable = imx_pwm_set_enable_v2,
.pwm_ops = &imx_pwm_ops_v2,
};
@@ -406,9 +291,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
if (IS_ERR(imx->mmio_base))
return PTR_ERR(imx->mmio_base);
- imx->config = data->config;
- imx->set_enable = data->set_enable;
-
ret = pwmchip_add(&imx->chip);
if (ret < 0)
return ret;
--
2.1.4
This patch set brings atomic operation to i.MX's PWMv2 driver.
This work has been supported and suggested by Boris Brezillon [1] and
Stefan Agner, by showing how simple the transition could be :-).
It has been divided into several steps:
- Remove ipg clock enable/disable code (as proposed by Sascha Hauer) - this
is the most notable change for v3
- Provide different pwm ops for PWMv1 and PWMv2
- Separate PWMv1 commits from "generic" and non atomic PWM code
NOTE: Since I do _not_ have board with PWMv1, I would like to ask somebody
for testing
- Move some imx_config_v2 code to separate functions
- Provide PWM atomic implementation (the ->apply() driver) for PWMv2 in a
single patch for better readability.
- Remove redundant PWM code (disable, enable, config callbacks)
- Update proper documentation entries
- Provide support for polarity inversion on top of atomic PWM rework
Test HW:
--------
This patch set has been tested on i.MX6q board with
v4.9 vanila kernel SHA1: 69973b830859bc6529a7a0468ba0d80ee5117826
The PWM operation has been tested with pwm_bl backlight driver by changing
its brightness.
[1]: http://patchwork.ozlabs.org/patch/685402/
Lothar Wassmann (2):
pwm: print error messages with pr_err() instead of pr_debug()
pwm: core: make the PWM_POLARITY flag in DTB optional
Lukasz Majewski (8):
pwm: imx: Add separate set of pwm ops for PWMv1 and PWMv2
pwm: imx: Rewrite imx_pwm_*_v1 code to facilitate switch to atomic pwm
operation
pwm: imx: Move PWMv2 software reset code to a separate function
pwm: imx: Move PWMv2 wait for fifo slot code to a separate function
pwm: imx: Provide atomic PWM support for i.MX PWMv2
pwm: imx: Remove redundant i.MX PWMv2 code
pwm: imx: doc: Update imx-pwm.txt documentation entry
pwm: imx: Add polarity inversion support to i.MX's PWMv2
Sascha Hauer (1):
pwm: imx: remove ipg clock
Documentation/devicetree/bindings/pwm/imx-pwm.txt | 6 +-
drivers/pwm/core.c | 26 +--
drivers/pwm/pwm-imx.c | 247 ++++++++++------------
3 files changed, 134 insertions(+), 145 deletions(-)
--
2.1.4
With this patch the polarity settings for i.MX's PWMv2 is now supported
on top of atomic PWM setting
Signed-off-by: Bhuvanchandra DV <[email protected]>
Signed-off-by: Lukasz Majewski <[email protected]>
---
Changes for v3:
- None
Changes for v2:
- New patch
---
drivers/pwm/pwm-imx.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index b636526..03b01a4 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -38,6 +38,7 @@
#define MX3_PWMCR_DOZEEN (1 << 24)
#define MX3_PWMCR_WAITEN (1 << 23)
#define MX3_PWMCR_DBGEN (1 << 22)
+#define MX3_PWMCR_POUTC (1 << 18)
#define MX3_PWMCR_CLKSRC_IPG_HIGH (2 << 16)
#define MX3_PWMCR_CLKSRC_IPG (1 << 16)
#define MX3_PWMCR_SWR (1 << 3)
@@ -215,6 +216,9 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
if (state->enabled)
cr |= MX3_PWMCR_EN;
+ if (state->polarity == PWM_POLARITY_INVERSED)
+ cr |= MX3_PWMCR_POUTC;
+
writel(cr, imx->mmio_base + MX3_PWMCR);
/* Disable the clock if the PWM is being disabled. */
@@ -237,6 +241,7 @@ static struct pwm_ops imx_pwm_ops_v2 = {
};
struct imx_pwm_data {
+ bool polarity_supported;
struct pwm_ops *pwm_ops;
};
@@ -245,6 +250,7 @@ static struct imx_pwm_data imx_pwm_data_v1 = {
};
static struct imx_pwm_data imx_pwm_data_v2 = {
+ .polarity_supported = true,
.pwm_ops = &imx_pwm_ops_v2,
};
@@ -285,6 +291,11 @@ static int imx_pwm_probe(struct platform_device *pdev)
imx->chip.base = -1;
imx->chip.npwm = 1;
imx->chip.can_sleep = true;
+ if (data->polarity_supported) {
+ dev_dbg(&pdev->dev, "PWM supports output inversion\n");
+ imx->chip.of_xlate = of_pwm_xlate_with_flags;
+ imx->chip.of_pwm_n_cells = 3;
+ }
r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
imx->mmio_base = devm_ioremap_resource(&pdev->dev, r);
--
2.1.4
On 2016-12-26 23:55, Lukasz Majewski wrote:
> From: Sascha Hauer <[email protected]>
>
> The use of the ipg clock was introduced with commit 7b27c160c681
> ("pwm: i.MX: fix clock lookup").
> In the commit message it was claimed that the ipg clock is enabled for
> register accesses. This is true for the ->config() callback, but not
> for the ->set_enable() callback. Given that the ipg clock is not
> consistently enabled for all register accesses we can assume that either
> it is not required at all or that the current code does not work.
> Remove the ipg clock code for now so that it's no longer in the way of
> refactoring the driver.
Hi Lukasz,
Has my concern addressed in any way with this resend?
https://lkml.org/lkml/2016/11/22/729
Breaking hardware is usually not an option :-)
I checked the i.MX 7 reference manual again, and in this case the
peripheral access clock is a clock line named "ipg_clk_s" (Table 12-20),
with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 all clocks are
behind a single gate, so in fact it does not matter which clock we take.
Given that others have peripheral access behind the "pwm" gate, I guess
we should take the "pwm" gate...
--
Stefan
>
> Signed-off-by: Sascha Hauer <[email protected]>
> Cc: Philipp Zabel <[email protected]>
> ---
> [commit message text refactored by Lukasz Majewski <[email protected]>]
> ---
> Changes for v3:
> - New patch
> ---
> drivers/pwm/pwm-imx.c | 19 +------------------
> 1 file changed, 1 insertion(+), 18 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index d600fd5..70609ef2 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -49,7 +49,6 @@
>
> struct imx_chip {
> struct clk *clk_per;
> - struct clk *clk_ipg;
>
> void __iomem *mmio_base;
>
> @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> struct imx_chip *imx = to_imx_chip(chip);
> - int ret;
> -
> - ret = clk_prepare_enable(imx->clk_ipg);
> - if (ret)
> - return ret;
>
> - ret = imx->config(chip, pwm, duty_ns, period_ns);
> -
> - clk_disable_unprepare(imx->clk_ipg);
> -
> - return ret;
> + return imx->config(chip, pwm, duty_ns, period_ns);
> }
>
> static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct platform_device *pdev)
> return PTR_ERR(imx->clk_per);
> }
>
> - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> - if (IS_ERR(imx->clk_ipg)) {
> - dev_err(&pdev->dev, "getting ipg clock failed with %ld\n",
> - PTR_ERR(imx->clk_ipg));
> - return PTR_ERR(imx->clk_ipg);
> - }
> -
> imx->chip.ops = &imx_pwm_ops;
> imx->chip.dev = &pdev->dev;
> imx->chip.base = -1;
Hi Stefan,
> On 2016-12-26 23:55, Lukasz Majewski wrote:
> > From: Sascha Hauer <[email protected]>
> >
> > The use of the ipg clock was introduced with commit 7b27c160c681
> > ("pwm: i.MX: fix clock lookup").
> > In the commit message it was claimed that the ipg clock is enabled
> > for register accesses. This is true for the ->config() callback,
> > but not for the ->set_enable() callback. Given that the ipg clock
> > is not consistently enabled for all register accesses we can assume
> > that either it is not required at all or that the current code does
> > not work. Remove the ipg clock code for now so that it's no longer
> > in the way of refactoring the driver.
>
> Hi Lukasz,
>
> Has my concern addressed in any way with this resend?
> https://lkml.org/lkml/2016/11/22/729
Unfortunately not, since I don't have iMX7 for testing.
>
> Breaking hardware is usually not an option :-)
Yes, I know, but
Please look on the patch set from my perspective:
I originally wanted to add polarity inversion to PWM. Then, there was
the request from you and Boris to go with "atomicity" support, so I
converted the driver to support it.
This patch set has been resent on purpose at the end of merge window,
so we do have some time to fix it if it would be accepted to -next
tree (or any other PWM related one). Moreover, the burden for preparing
patches would be smaller - since we all have agreed that "atomicity" is
a more than welcome feature.
>
> I checked the i.MX 7 reference manual again, and in this case the
> peripheral access clock is a clock line named "ipg_clk_s" (Table
> 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7 all
> clocks are behind a single gate, so in fact it does not matter which
> clock we take. Given that others have peripheral access behind the
> "pwm" gate, I guess we should take the "pwm" gate...
If possible please prepare a patch. It would be the best solution.
Thanks in advance,
Łukasz Majewski
>
> --
> Stefan
>
> >
> > Signed-off-by: Sascha Hauer <[email protected]>
> > Cc: Philipp Zabel <[email protected]>
> > ---
> > [commit message text refactored by Lukasz Majewski
> > <[email protected]>] ---
> > Changes for v3:
> > - New patch
> > ---
> > drivers/pwm/pwm-imx.c | 19 +------------------
> > 1 file changed, 1 insertion(+), 18 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index d600fd5..70609ef2 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -49,7 +49,6 @@
> >
> > struct imx_chip {
> > struct clk *clk_per;
> > - struct clk *clk_ipg;
> >
> > void __iomem *mmio_base;
> >
> > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> > {
> > struct imx_chip *imx = to_imx_chip(chip);
> > - int ret;
> > -
> > - ret = clk_prepare_enable(imx->clk_ipg);
> > - if (ret)
> > - return ret;
> >
> > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > -
> > - clk_disable_unprepare(imx->clk_ipg);
> > -
> > - return ret;
> > + return imx->config(chip, pwm, duty_ns, period_ns);
> > }
> >
> > static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device
> > *pwm) @@ -293,13 +283,6 @@ static int imx_pwm_probe(struct
> > platform_device *pdev) return PTR_ERR(imx->clk_per);
> > }
> >
> > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > - if (IS_ERR(imx->clk_ipg)) {
> > - dev_err(&pdev->dev, "getting ipg clock failed with
> > %ld\n",
> > - PTR_ERR(imx->clk_ipg));
> > - return PTR_ERR(imx->clk_ipg);
> > - }
> > -
> > imx->chip.ops = &imx_pwm_ops;
> > imx->chip.dev = &pdev->dev;
> > imx->chip.base = -1;
Hi Stefan,
> Hi Stefan,
>
> > On 2016-12-26 23:55, Lukasz Majewski wrote:
> > > From: Sascha Hauer <[email protected]>
> > >
> > > The use of the ipg clock was introduced with commit 7b27c160c681
> > > ("pwm: i.MX: fix clock lookup").
> > > In the commit message it was claimed that the ipg clock is enabled
> > > for register accesses. This is true for the ->config() callback,
> > > but not for the ->set_enable() callback. Given that the ipg clock
> > > is not consistently enabled for all register accesses we can
> > > assume that either it is not required at all or that the current
> > > code does not work. Remove the ipg clock code for now so that
> > > it's no longer in the way of refactoring the driver.
> >
> > Hi Lukasz,
> >
> > Has my concern addressed in any way with this resend?
> > https://lkml.org/lkml/2016/11/22/729
>
> Unfortunately not, since I don't have iMX7 for testing.
>
> >
> > Breaking hardware is usually not an option :-)
>
> Yes, I know, but
>
> Please look on the patch set from my perspective:
>
> I originally wanted to add polarity inversion to PWM. Then, there was
> the request from you and Boris to go with "atomicity" support, so I
> converted the driver to support it.
>
> This patch set has been resent on purpose at the end of merge window,
> so we do have some time to fix it if it would be accepted to -next
> tree (or any other PWM related one). Moreover, the burden for
> preparing patches would be smaller - since we all have agreed that
> "atomicity" is a more than welcome feature.
>
>
> >
> > I checked the i.MX 7 reference manual again, and in this case the
> > peripheral access clock is a clock line named "ipg_clk_s" (Table
> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
> > all clocks are behind a single gate, so in fact it does not matter
> > which clock we take. Given that others have peripheral access
> > behind the "pwm" gate, I guess we should take the "pwm" gate...
>
>
> If possible please prepare a patch. It would be the best solution.
If I might ask - are you willing to prepare patch to fix iMX7 or shall
I roll back to the ipg code already present in main line ?
Best regards,
Łukasz Majewski
>
> Thanks in advance,
> Łukasz Majewski
>
> >
> > --
> > Stefan
> >
> > >
> > > Signed-off-by: Sascha Hauer <[email protected]>
> > > Cc: Philipp Zabel <[email protected]>
> > > ---
> > > [commit message text refactored by Lukasz Majewski
> > > <[email protected]>] ---
> > > Changes for v3:
> > > - New patch
> > > ---
> > > drivers/pwm/pwm-imx.c | 19 +------------------
> > > 1 file changed, 1 insertion(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index d600fd5..70609ef2 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -49,7 +49,6 @@
> > >
> > > struct imx_chip {
> > > struct clk *clk_per;
> > > - struct clk *clk_ipg;
> > >
> > > void __iomem *mmio_base;
> > >
> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> > > {
> > > struct imx_chip *imx = to_imx_chip(chip);
> > > - int ret;
> > > -
> > > - ret = clk_prepare_enable(imx->clk_ipg);
> > > - if (ret)
> > > - return ret;
> > >
> > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> > > -
> > > - clk_disable_unprepare(imx->clk_ipg);
> > > -
> > > - return ret;
> > > + return imx->config(chip, pwm, duty_ns, period_ns);
> > > }
> > >
> > > static int imx_pwm_enable(struct pwm_chip *chip, struct
> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
> > > imx_pwm_probe(struct platform_device *pdev) return
> > > PTR_ERR(imx->clk_per); }
> > >
> > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> > > - if (IS_ERR(imx->clk_ipg)) {
> > > - dev_err(&pdev->dev, "getting ipg clock failed
> > > with %ld\n",
> > > - PTR_ERR(imx->clk_ipg));
> > > - return PTR_ERR(imx->clk_ipg);
> > > - }
> > > -
> > > imx->chip.ops = &imx_pwm_ops;
> > > imx->chip.dev = &pdev->dev;
> > > imx->chip.base = -1;
>
On 2016-12-28 23:01, Lukasz Majewski wrote:
> Hi Stefan,
>
>> Hi Stefan,
>>
>> > On 2016-12-26 23:55, Lukasz Majewski wrote:
>> > > From: Sascha Hauer <[email protected]>
>> > >
>> > > The use of the ipg clock was introduced with commit 7b27c160c681
>> > > ("pwm: i.MX: fix clock lookup").
>> > > In the commit message it was claimed that the ipg clock is enabled
>> > > for register accesses. This is true for the ->config() callback,
>> > > but not for the ->set_enable() callback. Given that the ipg clock
>> > > is not consistently enabled for all register accesses we can
>> > > assume that either it is not required at all or that the current
>> > > code does not work. Remove the ipg clock code for now so that
>> > > it's no longer in the way of refactoring the driver.
>> >
>> > Hi Lukasz,
>> >
>> > Has my concern addressed in any way with this resend?
>> > https://lkml.org/lkml/2016/11/22/729
>>
>> Unfortunately not, since I don't have iMX7 for testing.
>>
>> >
>> > Breaking hardware is usually not an option :-)
>>
>> Yes, I know, but
>>
>> Please look on the patch set from my perspective:
>>
>> I originally wanted to add polarity inversion to PWM. Then, there was
>> the request from you and Boris to go with "atomicity" support, so I
>> converted the driver to support it.
>>
>> This patch set has been resent on purpose at the end of merge window,
>> so we do have some time to fix it if it would be accepted to -next
>> tree (or any other PWM related one). Moreover, the burden for
>> preparing patches would be smaller - since we all have agreed that
>> "atomicity" is a more than welcome feature.
>>
>>
>> >
>> > I checked the i.MX 7 reference manual again, and in this case the
>> > peripheral access clock is a clock line named "ipg_clk_s" (Table
>> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
>> > all clocks are behind a single gate, so in fact it does not matter
>> > which clock we take. Given that others have peripheral access
>> > behind the "pwm" gate, I guess we should take the "pwm" gate...
>>
>>
>> If possible please prepare a patch. It would be the best solution.
>
> If I might ask - are you willing to prepare patch to fix iMX7 or shall
> I roll back to the ipg code already present in main line ?
I doubt that just rolling back the existing code works for the new
atomic change... I guess we have to really introduce a clk enable on
each register access, also for the new atomic code.
Not sure if we should fold this change in a existing commit or create a
new patch ontop of your changes. The latter is probably easier, but
creates a "window of brokenness" for i.MX 7 in git history. But then, I
don't really mind since it currently works more or less by chance...
I can prepare a patch.
--
Stefan
>
>
> Best regards,
> Łukasz Majewski
>
>>
>> Thanks in advance,
>> Łukasz Majewski
>>
>> >
>> > --
>> > Stefan
>> >
>> > >
>> > > Signed-off-by: Sascha Hauer <[email protected]>
>> > > Cc: Philipp Zabel <[email protected]>
>> > > ---
>> > > [commit message text refactored by Lukasz Majewski
>> > > <[email protected]>] ---
>> > > Changes for v3:
>> > > - New patch
>> > > ---
>> > > drivers/pwm/pwm-imx.c | 19 +------------------
>> > > 1 file changed, 1 insertion(+), 18 deletions(-)
>> > >
>> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
>> > > index d600fd5..70609ef2 100644
>> > > --- a/drivers/pwm/pwm-imx.c
>> > > +++ b/drivers/pwm/pwm-imx.c
>> > > @@ -49,7 +49,6 @@
>> > >
>> > > struct imx_chip {
>> > > struct clk *clk_per;
>> > > - struct clk *clk_ipg;
>> > >
>> > > void __iomem *mmio_base;
>> > >
>> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
>> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
>> > > {
>> > > struct imx_chip *imx = to_imx_chip(chip);
>> > > - int ret;
>> > > -
>> > > - ret = clk_prepare_enable(imx->clk_ipg);
>> > > - if (ret)
>> > > - return ret;
>> > >
>> > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
>> > > -
>> > > - clk_disable_unprepare(imx->clk_ipg);
>> > > -
>> > > - return ret;
>> > > + return imx->config(chip, pwm, duty_ns, period_ns);
>> > > }
>> > >
>> > > static int imx_pwm_enable(struct pwm_chip *chip, struct
>> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
>> > > imx_pwm_probe(struct platform_device *pdev) return
>> > > PTR_ERR(imx->clk_per); }
>> > >
>> > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
>> > > - if (IS_ERR(imx->clk_ipg)) {
>> > > - dev_err(&pdev->dev, "getting ipg clock failed
>> > > with %ld\n",
>> > > - PTR_ERR(imx->clk_ipg));
>> > > - return PTR_ERR(imx->clk_ipg);
>> > > - }
>> > > -
>> > > imx->chip.ops = &imx_pwm_ops;
>> > > imx->chip.dev = &pdev->dev;
>> > > imx->chip.base = -1;
>>
Hi Stefan,
> On 2016-12-28 23:01, Lukasz Majewski wrote:
> > Hi Stefan,
> >
> >> Hi Stefan,
> >>
> >> > On 2016-12-26 23:55, Lukasz Majewski wrote:
> >> > > From: Sascha Hauer <[email protected]>
> >> > >
> >> > > The use of the ipg clock was introduced with commit
> >> > > 7b27c160c681 ("pwm: i.MX: fix clock lookup").
> >> > > In the commit message it was claimed that the ipg clock is
> >> > > enabled for register accesses. This is true for the ->config()
> >> > > callback, but not for the ->set_enable() callback. Given that
> >> > > the ipg clock is not consistently enabled for all register
> >> > > accesses we can assume that either it is not required at all
> >> > > or that the current code does not work. Remove the ipg clock
> >> > > code for now so that it's no longer in the way of refactoring
> >> > > the driver.
> >> >
> >> > Hi Lukasz,
> >> >
> >> > Has my concern addressed in any way with this resend?
> >> > https://lkml.org/lkml/2016/11/22/729
> >>
> >> Unfortunately not, since I don't have iMX7 for testing.
> >>
> >> >
> >> > Breaking hardware is usually not an option :-)
> >>
> >> Yes, I know, but
> >>
> >> Please look on the patch set from my perspective:
> >>
> >> I originally wanted to add polarity inversion to PWM. Then, there
> >> was the request from you and Boris to go with "atomicity" support,
> >> so I converted the driver to support it.
> >>
> >> This patch set has been resent on purpose at the end of merge
> >> window, so we do have some time to fix it if it would be accepted
> >> to -next tree (or any other PWM related one). Moreover, the burden
> >> for preparing patches would be smaller - since we all have agreed
> >> that "atomicity" is a more than welcome feature.
> >>
> >>
> >> >
> >> > I checked the i.MX 7 reference manual again, and in this case the
> >> > peripheral access clock is a clock line named "ipg_clk_s" (Table
> >> > 12-20), with a clock root "PWM1_CLK_ROOT" (Table 5-12). In i.MX 7
> >> > all clocks are behind a single gate, so in fact it does not
> >> > matter which clock we take. Given that others have peripheral
> >> > access behind the "pwm" gate, I guess we should take the "pwm"
> >> > gate...
> >>
> >>
> >> If possible please prepare a patch. It would be the best solution.
> >
> > If I might ask - are you willing to prepare patch to fix iMX7 or
> > shall I roll back to the ipg code already present in main line ?
>
> I doubt that just rolling back the existing code works for the new
> atomic change... I guess we have to really introduce a clk enable on
> each register access, also for the new atomic code.
Maybe this is a way to go.
>
> Not sure if we should fold this change in a existing commit or create
> a new patch ontop of your changes. The latter is probably easier,
I would opt for creating (adding) changes on top of my changes.
> but
> creates a "window of brokenness" for i.MX 7 in git history. But then,
> I don't really mind since it currently works more or less by chance...
Hmm... it seems like Sascha patch (included to the patch series):
http://patchwork.ozlabs.org/patch/708847/
is a good starting point (at least we removed ipg clock) for fixing
things.
>
> I can prepare a patch.
Thank you :-)
Best regards,
Łukasz Majewski
>
> --
> Stefan
>
> >
> >
> > Best regards,
> > Łukasz Majewski
> >
> >>
> >> Thanks in advance,
> >> Łukasz Majewski
> >>
> >> >
> >> > --
> >> > Stefan
> >> >
> >> > >
> >> > > Signed-off-by: Sascha Hauer <[email protected]>
> >> > > Cc: Philipp Zabel <[email protected]>
> >> > > ---
> >> > > [commit message text refactored by Lukasz Majewski
> >> > > <[email protected]>] ---
> >> > > Changes for v3:
> >> > > - New patch
> >> > > ---
> >> > > drivers/pwm/pwm-imx.c | 19 +------------------
> >> > > 1 file changed, 1 insertion(+), 18 deletions(-)
> >> > >
> >> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> >> > > index d600fd5..70609ef2 100644
> >> > > --- a/drivers/pwm/pwm-imx.c
> >> > > +++ b/drivers/pwm/pwm-imx.c
> >> > > @@ -49,7 +49,6 @@
> >> > >
> >> > > struct imx_chip {
> >> > > struct clk *clk_per;
> >> > > - struct clk *clk_ipg;
> >> > >
> >> > > void __iomem *mmio_base;
> >> > >
> >> > > @@ -204,17 +203,8 @@ static int imx_pwm_config(struct pwm_chip
> >> > > *chip, struct pwm_device *pwm, int duty_ns, int period_ns)
> >> > > {
> >> > > struct imx_chip *imx = to_imx_chip(chip);
> >> > > - int ret;
> >> > > -
> >> > > - ret = clk_prepare_enable(imx->clk_ipg);
> >> > > - if (ret)
> >> > > - return ret;
> >> > >
> >> > > - ret = imx->config(chip, pwm, duty_ns, period_ns);
> >> > > -
> >> > > - clk_disable_unprepare(imx->clk_ipg);
> >> > > -
> >> > > - return ret;
> >> > > + return imx->config(chip, pwm, duty_ns, period_ns);
> >> > > }
> >> > >
> >> > > static int imx_pwm_enable(struct pwm_chip *chip, struct
> >> > > pwm_device *pwm) @@ -293,13 +283,6 @@ static int
> >> > > imx_pwm_probe(struct platform_device *pdev) return
> >> > > PTR_ERR(imx->clk_per); }
> >> > >
> >> > > - imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg");
> >> > > - if (IS_ERR(imx->clk_ipg)) {
> >> > > - dev_err(&pdev->dev, "getting ipg clock failed
> >> > > with %ld\n",
> >> > > - PTR_ERR(imx->clk_ipg));
> >> > > - return PTR_ERR(imx->clk_ipg);
> >> > > - }
> >> > > -
> >> > > imx->chip.ops = &imx_pwm_ops;
> >> > > imx->chip.dev = &pdev->dev;
> >> > > imx->chip.base = -1;
> >>
Hi Lukasz,
On Mon, 26 Dec 2016 23:55:57 +0100
Lukasz Majewski <[email protected]> wrote:
> This commit provides apply() callback implementation for i.MX's PWMv2.
>
> Suggested-by: Stefan Agner <[email protected]>
> Suggested-by: Boris Brezillon <[email protected]>
> Signed-off-by: Lukasz Majewski <[email protected]>
> Reviewed-by: Boris Brezillon <[email protected]>
> ---
> Changes for v3:
> - Remove ipg clock enable/disable functions
>
> Changes for v2:
> - None
> ---
> drivers/pwm/pwm-imx.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 70 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index ebe9b0c..cd53c05 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> }
> }
>
> +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + unsigned long period_cycles, duty_cycles, prescale;
> + struct imx_chip *imx = to_imx_chip(chip);
> + struct pwm_state cstate;
> + unsigned long long c;
> + u32 cr = 0;
> + int ret;
> +
> + pwm_get_state(pwm, &cstate);
> +
> + c = clk_get_rate(imx->clk_per);
> + c *= state->period;
> +
> + do_div(c, 1000000000);
> + period_cycles = c;
> +
> + prescale = period_cycles / 0x10000 + 1;
> +
> + period_cycles /= prescale;
> + c = (unsigned long long)period_cycles * state->duty_cycle;
> + do_div(c, state->period);
> + duty_cycles = c;
> +
> + /*
> + * according to imx pwm RM, the real period value should be
> + * PERIOD value in PWMPR plus 2.
> + */
> + if (period_cycles > 2)
> + period_cycles -= 2;
> + else
> + period_cycles = 0;
> +
> + /* Enable the clock if the PWM is being enabled. */
> + if (state->enabled && !cstate.enabled) {
> + ret = clk_prepare_enable(imx->clk_per);
> + if (ret)
> + return ret;
> + }
> +
> + /*
> + * Wait for a free FIFO slot if the PWM is already enabled, and flush
> + * the FIFO if the PWM was disabled and is about to be enabled.
> + */
> + if (cstate.enabled)
> + imx_pwm_wait_fifo_slot(chip, pwm);
> + else if (state->enabled)
> + imx_pwm_sw_reset(chip);
> +
> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> + writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> +
> + cr |= MX3_PWMCR_PRESCALER(prescale) |
> + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> +
> + if (state->enabled)
> + cr |= MX3_PWMCR_EN;
> +
> + writel(cr, imx->mmio_base + MX3_PWMCR);
> +
> + /* Disable the clock if the PWM is being disabled. */
> + if (!state->enabled && cstate.enabled)
> + clk_disable_unprepare(imx->clk_per);
> +
> + return 0;
> +}
> +
Stefan suggested to rework this function to avoid unneeded
duty/period calculation and reg write when disabling the PWM. Why
didn't you send a v4 addressing that instead of resending the exact
same v3?
Same goes for the regression introduced in patch 2: I think it's better
to keep things bisectable on all platforms (even if it appeared to work
by chance on imx7, it did work before this change).
That's just my opinion, but when you get reviews on a patch series, it's
better to address them directly (especially when issues can be easily
fixed) than provide follow-up patches.
Regards,
Boris
Hi Boris,
> Hi Lukasz,
>
> On Mon, 26 Dec 2016 23:55:57 +0100
> Lukasz Majewski <[email protected]> wrote:
>
> > This commit provides apply() callback implementation for i.MX's
> > PWMv2.
> >
> > Suggested-by: Stefan Agner <[email protected]>
> > Suggested-by: Boris Brezillon <[email protected]>
> > Signed-off-by: Lukasz Majewski <[email protected]>
> > Reviewed-by: Boris Brezillon <[email protected]>
> > ---
> > Changes for v3:
> > - Remove ipg clock enable/disable functions
> >
> > Changes for v2:
> > - None
> > ---
> > drivers/pwm/pwm-imx.c | 70
> > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > 70 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > index ebe9b0c..cd53c05 100644
> > --- a/drivers/pwm/pwm-imx.c
> > +++ b/drivers/pwm/pwm-imx.c
> > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > pwm_chip *chip, }
> > }
> >
> > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + unsigned long period_cycles, duty_cycles, prescale;
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + struct pwm_state cstate;
> > + unsigned long long c;
> > + u32 cr = 0;
> > + int ret;
> > +
> > + pwm_get_state(pwm, &cstate);
> > +
> > + c = clk_get_rate(imx->clk_per);
> > + c *= state->period;
> > +
> > + do_div(c, 1000000000);
> > + period_cycles = c;
> > +
> > + prescale = period_cycles / 0x10000 + 1;
> > +
> > + period_cycles /= prescale;
> > + c = (unsigned long long)period_cycles * state->duty_cycle;
> > + do_div(c, state->period);
> > + duty_cycles = c;
> > +
> > + /*
> > + * according to imx pwm RM, the real period value should be
> > + * PERIOD value in PWMPR plus 2.
> > + */
> > + if (period_cycles > 2)
> > + period_cycles -= 2;
> > + else
> > + period_cycles = 0;
> > +
> > + /* Enable the clock if the PWM is being enabled. */
> > + if (state->enabled && !cstate.enabled) {
> > + ret = clk_prepare_enable(imx->clk_per);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + /*
> > + * Wait for a free FIFO slot if the PWM is already
> > enabled, and flush
> > + * the FIFO if the PWM was disabled and is about to be
> > enabled.
> > + */
> > + if (cstate.enabled)
> > + imx_pwm_wait_fifo_slot(chip, pwm);
> > + else if (state->enabled)
> > + imx_pwm_sw_reset(chip);
> > +
> > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > + writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > +
> > + cr |= MX3_PWMCR_PRESCALER(prescale) |
> > + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > +
> > + if (state->enabled)
> > + cr |= MX3_PWMCR_EN;
> > +
> > + writel(cr, imx->mmio_base + MX3_PWMCR);
> > +
> > + /* Disable the clock if the PWM is being disabled. */
> > + if (!state->enabled && cstate.enabled)
> > + clk_disable_unprepare(imx->clk_per);
> > +
> > + return 0;
> > +}
> > +
>
> Stefan suggested to rework this function to avoid unneeded
> duty/period calculation and reg write when disabling the PWM. Why
> didn't you send a v4 addressing that instead of resending the exact
> same v3?
The discussion between you and Stefan was in this thread:
http://patchwork.ozlabs.org/patch/689790/
Stefan proposed change, you replied with your concerns and that is all.
No clear decision what to change until today when Stefan prepared
separate (concise) patch (now I see what is the problem).
>
> Same goes for the regression introduced in patch 2: I think it's
> better to keep things bisectable on all platforms (even if it
> appeared to work by chance on imx7, it did work before this change).
Could you be more specific about your idea to solve this problem?
>
> That's just my opinion, but when you get reviews on a patch series,
> it's better to address them directly (especially when issues can be
> easily fixed) than provide follow-up patches.
I do not have iMX7 for testing/development, so I could not reproduce
the error and address the issue directly.
I can at best integrate Stefan's patch and hope to not introduce
regression.
Best regards,
Łukasz Majewski
>
> Regards,
>
> Boris
Hi Lukasz,
On Thu, 29 Dec 2016 17:45:35 +0100
Lukasz Majewski <[email protected]> wrote:
> Hi Boris,
>
> > Hi Lukasz,
> >
> > On Mon, 26 Dec 2016 23:55:57 +0100
> > Lukasz Majewski <[email protected]> wrote:
> >
> > > This commit provides apply() callback implementation for i.MX's
> > > PWMv2.
> > >
> > > Suggested-by: Stefan Agner <[email protected]>
> > > Suggested-by: Boris Brezillon <[email protected]>
> > > Signed-off-by: Lukasz Majewski <[email protected]>
> > > Reviewed-by: Boris Brezillon <[email protected]>
> > > ---
> > > Changes for v3:
> > > - Remove ipg clock enable/disable functions
> > >
> > > Changes for v2:
> > > - None
> > > ---
> > > drivers/pwm/pwm-imx.c | 70
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > 70 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> > > index ebe9b0c..cd53c05 100644
> > > --- a/drivers/pwm/pwm-imx.c
> > > +++ b/drivers/pwm/pwm-imx.c
> > > @@ -159,6 +159,75 @@ static void imx_pwm_wait_fifo_slot(struct
> > > pwm_chip *chip, }
> > > }
> > >
> > > +static int imx_pwm_apply_v2(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > + struct pwm_state *state)
> > > +{
> > > + unsigned long period_cycles, duty_cycles, prescale;
> > > + struct imx_chip *imx = to_imx_chip(chip);
> > > + struct pwm_state cstate;
> > > + unsigned long long c;
> > > + u32 cr = 0;
> > > + int ret;
> > > +
> > > + pwm_get_state(pwm, &cstate);
> > > +
> > > + c = clk_get_rate(imx->clk_per);
> > > + c *= state->period;
> > > +
> > > + do_div(c, 1000000000);
> > > + period_cycles = c;
> > > +
> > > + prescale = period_cycles / 0x10000 + 1;
> > > +
> > > + period_cycles /= prescale;
> > > + c = (unsigned long long)period_cycles * state->duty_cycle;
> > > + do_div(c, state->period);
> > > + duty_cycles = c;
> > > +
> > > + /*
> > > + * according to imx pwm RM, the real period value should be
> > > + * PERIOD value in PWMPR plus 2.
> > > + */
> > > + if (period_cycles > 2)
> > > + period_cycles -= 2;
> > > + else
> > > + period_cycles = 0;
> > > +
> > > + /* Enable the clock if the PWM is being enabled. */
> > > + if (state->enabled && !cstate.enabled) {
> > > + ret = clk_prepare_enable(imx->clk_per);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + /*
> > > + * Wait for a free FIFO slot if the PWM is already
> > > enabled, and flush
> > > + * the FIFO if the PWM was disabled and is about to be
> > > enabled.
> > > + */
> > > + if (cstate.enabled)
> > > + imx_pwm_wait_fifo_slot(chip, pwm);
> > > + else if (state->enabled)
> > > + imx_pwm_sw_reset(chip);
> > > +
> > > + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> > > + writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> > > +
> > > + cr |= MX3_PWMCR_PRESCALER(prescale) |
> > > + MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> > > + MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH;
> > > +
> > > + if (state->enabled)
> > > + cr |= MX3_PWMCR_EN;
> > > +
> > > + writel(cr, imx->mmio_base + MX3_PWMCR);
> > > +
> > > + /* Disable the clock if the PWM is being disabled. */
> > > + if (!state->enabled && cstate.enabled)
> > > + clk_disable_unprepare(imx->clk_per);
> > > +
> > > + return 0;
> > > +}
> > > +
> >
> > Stefan suggested to rework this function to avoid unneeded
> > duty/period calculation and reg write when disabling the PWM. Why
> > didn't you send a v4 addressing that instead of resending the exact
> > same v3?
>
> The discussion between you and Stefan was in this thread:
> http://patchwork.ozlabs.org/patch/689790/
>
> Stefan proposed change, you replied with your concerns and that is all.
Well, regarding the imx_pwm_apply_v2() suggested by Stefan, I think we
both agreed that most of the code was unneeded when all we want to do
is disable the PWM.
My concern was more about the way PWM changes are applied (->apply()
returns before the change is actually applied), but I agreed that it
could be fixed later on (if other people think it's really needed),
since the existing code already handles it this way.
> No clear decision what to change until today when Stefan prepared
> separate (concise) patch (now I see what is the problem).
>
The patch proposed by Stefan is addressing a different problem: the
periph clock has to be enabled before accessing registers.
>
> >
> > Same goes for the regression introduced in patch 2: I think it's
> > better to keep things bisectable on all platforms (even if it
> > appeared to work by chance on imx7, it did work before this change).
>
> Could you be more specific about your idea to solve this problem?
Stefan already provided a patch, I just think it should be fixed before
patch 2 to avoid breaking bisectibility.
>
> >
> > That's just my opinion, but when you get reviews on a patch series,
> > it's better to address them directly (especially when issues can be
> > easily fixed) than provide follow-up patches.
>
> I do not have iMX7 for testing/development, so I could not reproduce
> the error and address the issue directly.
Well, the description made by Stefan seemed pretty clear to me: you
need to enable the periph clock before accessing PWM registers.
>
> I can at best integrate Stefan's patch and hope to not introduce
> regression.
You can ask others to test your own patches. In this case, just
clearly state that the patch is untested and that you'd like people
owning a specific platform to test it.
Regards,
Boris