LinuxLists.cc - [PATCH 1/3] pwm: imx: Sort include files

[permalink] [raw]

Subject: [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values

Use existing macros to define register fields instead of manually shifting
the bit masks. Also define some more register bits.

Signed-off-by: Michal Vokáč <[email protected]>
---
drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 58 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index bcbcac4..7a4907b 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -5,6 +5,8 @@
* Derived from pxa PWM driver by eric miao <[email protected]>
*/

+#include <linux/bitfield.h>
+#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/err.h>
@@ -23,7 +25,7 @@
#define MX1_PWMS 0x04 /* PWM Sample Register */
#define MX1_PWMP 0x08 /* PWM Period Register */

-#define MX1_PWMC_EN (1 << 4)
+#define MX1_PWMC_EN BIT(4)

/* i.MX27, i.MX31, i.MX35 share the same PWM function block: */

@@ -31,18 +33,53 @@
#define MX3_PWMSR 0x04 /* PWM Status Register */
#define MX3_PWMSAR 0x0C /* PWM Sample Register */
#define MX3_PWMPR 0x10 /* PWM Period Register */
-#define MX3_PWMCR_PRESCALER(x) ((((x) - 1) & 0xFFF) << 4)
-#define MX3_PWMCR_STOPEN (1 << 25)
-#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)
-#define MX3_PWMCR_EN (1 << 0)
-#define MX3_PWMSR_FIFOAV_4WORDS 0x4
-#define MX3_PWMSR_FIFOAV_MASK 0x7
+
+#define MX3_PWMCR_FWM GENMASK(27, 26)
+#define MX3_PWMCR_STOPEN BIT(25)
+#define MX3_PWMCR_DOZEN BIT(24)
+#define MX3_PWMCR_WAITEN BIT(23)
+#define MX3_PWMCR_DBGEN BIT(22)
+#define MX3_PWMCR_BCTR BIT(21)
+#define MX3_PWMCR_HCTR BIT(20)
+
+#define MX3_PWMCR_POUTC GENMASK(19, 18)
+#define MX3_PWMCR_POUTC_NORMAL 0
+#define MX3_PWMCR_POUTC_INVERTED 1
+#define MX3_PWMCR_POUTC_OFF 2
+
+#define MX3_PWMCR_CLKSRC GENMASK(17, 16)
+#define MX3_PWMCR_CLKSRC_OFF 0
+#define MX3_PWMCR_CLKSRC_IPG 1
+#define MX3_PWMCR_CLKSRC_IPG_HIGH 2
+#define MX3_PWMCR_CLKSRC_IPG_32K 3
+
+#define MX3_PWMCR_PRESCALER GENMASK(15, 4)
+
+#define MX3_PWMCR_SWR BIT(3)
+
+#define MX3_PWMCR_REPEAT GENMASK(2, 1)
+#define MX3_PWMCR_REPEAT_1X 0
+#define MX3_PWMCR_REPEAT_2X 1
+#define MX3_PWMCR_REPEAT_4X 2
+#define MX3_PWMCR_REPEAT_8X 3
+
+#define MX3_PWMCR_EN BIT(0)
+
+#define MX3_PWMSR_FWE BIT(6)
+#define MX3_PWMSR_CMP BIT(5)
+#define MX3_PWMSR_ROV BIT(4)
+#define MX3_PWMSR_FE BIT(3)
+
+#define MX3_PWMSR_FIFOAV GENMASK(2, 0)
+#define MX3_PWMSR_FIFOAV_EMPTY 0
+#define MX3_PWMSR_FIFOAV_1WORD 1
+#define MX3_PWMSR_FIFOAV_2WORDS 2
+#define MX3_PWMSR_FIFOAV_3WORDS 3
+#define MX3_PWMSR_FIFOAV_4WORDS 4
+
+#define MX3_PWMCR_PRESCALER_SET(x) FIELD_PREP(MX3_PWMCR_PRESCALER, (x) - 1)
+#define MX3_PWMCR_PRESCALER_GET(x) (FIELD_GET(MX3_PWMCR_PRESCALER, \
+ (x)) + 1)

#define MX3_PWM_SWR_LOOP 5

@@ -142,14 +179,14 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
u32 sr;

sr = readl(imx->mmio_base + MX3_PWMSR);
- fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
+ fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
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))
+ if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
dev_warn(dev, "there is no free FIFO slot\n");
}
}
@@ -207,13 +244,14 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
writel(period_cycles, imx->mmio_base + MX3_PWMPR);

- cr = MX3_PWMCR_PRESCALER(prescale) |
- MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
- MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
- MX3_PWMCR_EN;
+ cr = MX3_PWMCR_PRESCALER_SET(prescale) |
+ MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
+ FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
+ MX3_PWMCR_DBGEN | MX3_PWMCR_EN;

if (state->polarity == PWM_POLARITY_INVERSED)
- cr |= MX3_PWMCR_POUTC;
+ cr |= FIELD_PREP(MX3_PWMCR_POUTC,
+ MX3_PWMCR_POUTC_INVERTED);

writel(cr, imx->mmio_base + MX3_PWMCR);
} else if (cstate.enabled) {
--
2.1.4

2018-10-01 14:20:58

[permalink] [raw]

Subject: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

Implement the get_state() function and set the initial state to reflect
real state of the hardware. This allows to keep the PWM running if it was
enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
set as output in bootloader keep the same setting in Linux unless it is
reconfigured.

If we find the PWM block enabled we need to prepare and enable its source
clock otherwise the clock will be disabled late in the boot as unused.
That will leave the PWM in enabled state but with disabled clock. That has
a side effect that the PWM output is left at its current level at which
the clock was disabled. It is totally non-deterministic and it may be LOW
or HIGH.

Signed-off-by: Michal Vokáč <[email protected]>
---
drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)

diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
index 7a4907b..6cd3b72 100644
--- a/drivers/pwm/pwm-imx.c
+++ b/drivers/pwm/pwm-imx.c
@@ -83,6 +83,9 @@

#define MX3_PWM_SWR_LOOP 5

+/* PWMPR register value of 0xffff has the same effect as 0xfffe */
+#define MX3_PWMPR_MAX 0xfffe
+
struct imx_chip {
struct clk *clk_per;

@@ -93,6 +96,55 @@ struct imx_chip {

#define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)

+static void imx_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm, struct pwm_state *state)
+{
+ struct imx_chip *imx = to_imx_chip(chip);
+ u32 period, prescaler, pwm_clk, ret, val;
+ u64 tmp;
+
+ val = readl(imx->mmio_base + MX3_PWMCR);
+
+ if (val & MX3_PWMCR_EN) {
+ state->enabled = true;
+ ret = clk_prepare_enable(imx->clk_per);
+ if (ret)
+ return;
+ } else {
+ state->enabled = false;
+ }
+
+ switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
+ case MX3_PWMCR_POUTC_NORMAL:
+ state->polarity = PWM_POLARITY_NORMAL;
+ break;
+ case MX3_PWMCR_POUTC_INVERTED:
+ state->polarity = PWM_POLARITY_INVERSED;
+ break;
+ default:
+ dev_warn(chip->dev, "can't set polarity, output disconnected");
+ }
+
+ prescaler = MX3_PWMCR_PRESCALER_GET(val);
+ pwm_clk = clk_get_rate(imx->clk_per);
+ pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
+ val = readl(imx->mmio_base + MX3_PWMPR);
+ period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
+
+ /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
+ tmp = NSEC_PER_SEC * (u64)(period + 2);
+ state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
+
+ /* PWMSAR can be read only if PWM is enabled */
+ if (state->enabled) {
+ val = readl(imx->mmio_base + MX3_PWMSAR);
+ tmp = NSEC_PER_SEC * (u64)(val);
+ state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
+ } else {
+ state->duty_cycle = 0;
+ }
+}
+
static int imx_pwm_config_v1(struct pwm_chip *chip,
struct pwm_device *pwm, int duty_ns, int period_ns)
{
@@ -272,6 +324,7 @@ static const struct pwm_ops imx_pwm_ops_v1 = {

static const struct pwm_ops imx_pwm_ops_v2 = {
.apply = imx_pwm_apply_v2,
+ .get_state = imx_pwm_get_state,
.owner = THIS_MODULE,
};

--
2.1.4

2018-12-12 10:52:37

[permalink] [raw]

Subject: Re: [3/3] pwm: imx: Implement get_state() function for hardware readout

Hello,

On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> Implement the get_state() function and set the initial state to reflect
> real state of the hardware. This allows to keep the PWM running if it was
> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> set as output in bootloader keep the same setting in Linux unless it is
> reconfigured.
>
> If we find the PWM block enabled we need to prepare and enable its source
> clock otherwise the clock will be disabled late in the boot as unused.
> That will leave the PWM in enabled state but with disabled clock. That has
> a side effect that the PWM output is left at its current level at which
> the clock was disabled. It is totally non-deterministic and it may be LOW
> or HIGH.

Does this problem still exist if the pwm-imx driver is a module?

> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 7a4907b..6cd3b72 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -83,6 +83,9 @@
>
> #define MX3_PWM_SWR_LOOP 5
>
> +/* PWMPR register value of 0xffff has the same effect as 0xfffe */
> +#define MX3_PWMPR_MAX 0xfffe
> +
> struct imx_chip {
> struct clk *clk_per;
>
> @@ -93,6 +96,55 @@ struct imx_chip {
>
> #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
>
> +static void imx_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm, struct pwm_state *state)

broken alignment.

> +{
> + struct imx_chip *imx = to_imx_chip(chip);
> + u32 period, prescaler, pwm_clk, ret, val;
> + u64 tmp;
> +
> + val = readl(imx->mmio_base + MX3_PWMCR);
> +
> + if (val & MX3_PWMCR_EN) {
> + state->enabled = true;
> + ret = clk_prepare_enable(imx->clk_per);
> + if (ret)
> + return;
> + } else {
> + state->enabled = false;
> + }
> +
> + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> + case MX3_PWMCR_POUTC_NORMAL:
> + state->polarity = PWM_POLARITY_NORMAL;
> + break;
> + case MX3_PWMCR_POUTC_INVERTED:
> + state->polarity = PWM_POLARITY_INVERSED;
> + break;
> + default:
> + dev_warn(chip->dev, "can't set polarity, output disconnected");

Should we return an error here?

> + }
> +
> + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> + pwm_clk = clk_get_rate(imx->clk_per);
> + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> + val = readl(imx->mmio_base + MX3_PWMPR);

It would be more cautionous to not rely on the reserved bits to read as
zero. So I suggest to mask the value with 0xffff.

> + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> +
> + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> + tmp = NSEC_PER_SEC * (u64)(period + 2);
> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);

Would it make sense to introduce a policy about how to round in this
case? (Similarily for .apply?) This is of course out of scope for this
patch.

> +
> + /* PWMSAR can be read only if PWM is enabled */
> + if (state->enabled) {
> + val = readl(imx->mmio_base + MX3_PWMSAR);
> + tmp = NSEC_PER_SEC * (u64)(val);
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> + } else {
> + state->duty_cycle = 0;
> + }
> +}
> +
> static int imx_pwm_config_v1(struct pwm_chip *chip,
> struct pwm_device *pwm, int duty_ns, int period_ns)
> {
> @@ -272,6 +324,7 @@ static const struct pwm_ops imx_pwm_ops_v1 = {
>
> static const struct pwm_ops imx_pwm_ops_v2 = {
> .apply = imx_pwm_apply_v2,
> + .get_state = imx_pwm_get_state,
> .owner = THIS_MODULE,
> };
>

Attachments:

(No filename) (3.67 kB)
signature.asc (499.00 B)
Download all attachments

2018-12-12 10:53:52

[permalink] [raw]

Subject: Re: [PATCH 1/3] pwm: imx: Sort include files

On Mon, Oct 01, 2018 at 04:19:46PM +0200, Michal Vokáč wrote:
> Sort included header files alphabetically.
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)

Applied, thanks.

Thierry

Attachments:

(No filename) (307.00 B)
signature.asc (849.00 B)
Download all attachments

2018-12-12 10:55:22

[permalink] [raw]

Subject: Re: [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values

On Mon, Oct 01, 2018 at 04:19:47PM +0200, Michal Vokáč wrote:
> Use existing macros to define register fields instead of manually shifting
> the bit masks. Also define some more register bits.
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 58 insertions(+), 20 deletions(-)

Applied, thanks.

Thierry

Attachments:

(No filename) (435.00 B)
signature.asc (849.00 B)
Download all attachments

2018-12-12 10:55:43

[permalink] [raw]

Subject: Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> Implement the get_state() function and set the initial state to reflect
> real state of the hardware. This allows to keep the PWM running if it was
> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> set as output in bootloader keep the same setting in Linux unless it is
> reconfigured.
>
> If we find the PWM block enabled we need to prepare and enable its source
> clock otherwise the clock will be disabled late in the boot as unused.
> That will leave the PWM in enabled state but with disabled clock. That has
> a side effect that the PWM output is left at its current level at which
> the clock was disabled. It is totally non-deterministic and it may be LOW
> or HIGH.
>
> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)

Applied, thanks.

Thierry

Attachments:

(No filename) (0.98 kB)
signature.asc (849.00 B)
Download all attachments

2018-12-12 10:56:24

[permalink] [raw]

Subject: Re: [2/3] pwm: imx: Use bitops and bitfield macros to define register values

On Mon, Oct 01, 2018 at 04:19:47PM +0200, Michal Vokáč wrote:
> Use existing macros to define register fields instead of manually shifting
> the bit masks. Also define some more register bits.

I didn't check, but wonder if these additional register bits are then
used in the next patch. Maybe I'd change this patch to not introduce
something new, only let it modify the already existing stuff and then
introduce the new bits in the patch that makes use of them.

> Signed-off-by: Michal Vokáč <[email protected]>
> ---
> drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 58 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index bcbcac4..7a4907b 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -5,6 +5,8 @@
> * Derived from pxa PWM driver by eric miao <[email protected]>
> */
>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/err.h>
> @@ -23,7 +25,7 @@
> #define MX1_PWMS 0x04 /* PWM Sample Register */
> #define MX1_PWMP 0x08 /* PWM Period Register */
>
> -#define MX1_PWMC_EN (1 << 4)
> +#define MX1_PWMC_EN BIT(4)
>
> /* i.MX27, i.MX31, i.MX35 share the same PWM function block: */
>
> @@ -31,18 +33,53 @@
> #define MX3_PWMSR 0x04 /* PWM Status Register */
> #define MX3_PWMSAR 0x0C /* PWM Sample Register */
> #define MX3_PWMPR 0x10 /* PWM Period Register */
> -#define MX3_PWMCR_PRESCALER(x) ((((x) - 1) & 0xFFF) << 4)
> -#define MX3_PWMCR_STOPEN (1 << 25)
> -#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)
> -#define MX3_PWMCR_EN (1 << 0)
> -#define MX3_PWMSR_FIFOAV_4WORDS 0x4
> -#define MX3_PWMSR_FIFOAV_MASK 0x7
> +
> +#define MX3_PWMCR_FWM GENMASK(27, 26)
> +#define MX3_PWMCR_STOPEN BIT(25)
> +#define MX3_PWMCR_DOZEN BIT(24)
> +#define MX3_PWMCR_WAITEN BIT(23)
> +#define MX3_PWMCR_DBGEN BIT(22)
> +#define MX3_PWMCR_BCTR BIT(21)
> +#define MX3_PWMCR_HCTR BIT(20)
> +
> +#define MX3_PWMCR_POUTC GENMASK(19, 18)
> +#define MX3_PWMCR_POUTC_NORMAL 0
> +#define MX3_PWMCR_POUTC_INVERTED 1
> +#define MX3_PWMCR_POUTC_OFF 2
> +
> +#define MX3_PWMCR_CLKSRC GENMASK(17, 16)
> +#define MX3_PWMCR_CLKSRC_OFF 0
> +#define MX3_PWMCR_CLKSRC_IPG 1
> +#define MX3_PWMCR_CLKSRC_IPG_HIGH 2
> +#define MX3_PWMCR_CLKSRC_IPG_32K 3
> +
> +#define MX3_PWMCR_PRESCALER GENMASK(15, 4)
> +
> +#define MX3_PWMCR_SWR BIT(3)
> +
> +#define MX3_PWMCR_REPEAT GENMASK(2, 1)
> +#define MX3_PWMCR_REPEAT_1X 0
> +#define MX3_PWMCR_REPEAT_2X 1
> +#define MX3_PWMCR_REPEAT_4X 2
> +#define MX3_PWMCR_REPEAT_8X 3
> +
> +#define MX3_PWMCR_EN BIT(0)
> +
> +#define MX3_PWMSR_FWE BIT(6)
> +#define MX3_PWMSR_CMP BIT(5)
> +#define MX3_PWMSR_ROV BIT(4)
> +#define MX3_PWMSR_FE BIT(3)
> +
> +#define MX3_PWMSR_FIFOAV GENMASK(2, 0)
> +#define MX3_PWMSR_FIFOAV_EMPTY 0
> +#define MX3_PWMSR_FIFOAV_1WORD 1
> +#define MX3_PWMSR_FIFOAV_2WORDS 2
> +#define MX3_PWMSR_FIFOAV_3WORDS 3
> +#define MX3_PWMSR_FIFOAV_4WORDS 4
> +
> +#define MX3_PWMCR_PRESCALER_SET(x) FIELD_PREP(MX3_PWMCR_PRESCALER, (x) - 1)
> +#define MX3_PWMCR_PRESCALER_GET(x) (FIELD_GET(MX3_PWMCR_PRESCALER, \
> + (x)) + 1)

I wouldn't hide the +1 and -1 in a macro but as this was already the
case before your patch, that's ok.

> #define MX3_PWM_SWR_LOOP 5
>
> @@ -142,14 +179,14 @@ static void imx_pwm_wait_fifo_slot(struct pwm_chip *chip,
> u32 sr;
>
> sr = readl(imx->mmio_base + MX3_PWMSR);
> - fifoav = sr & MX3_PWMSR_FIFOAV_MASK;
> + fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> 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))
> + if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
> dev_warn(dev, "there is no free FIFO slot\n");
> }
> }
> @@ -207,13 +244,14 @@ static int imx_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
> writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
>
> - cr = MX3_PWMCR_PRESCALER(prescale) |
> - MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
> - MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC_IPG_HIGH |
> - MX3_PWMCR_EN;
> + cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> + MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> + FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> + MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
>
> if (state->polarity == PWM_POLARITY_INVERSED)
> - cr |= MX3_PWMCR_POUTC;
> + cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> + MX3_PWMCR_POUTC_INVERTED);
>
> writel(cr, imx->mmio_base + MX3_PWMCR);
> } else if (cstate.enabled) {

Best regards
Uwe

Attachments:

(No filename) (5.14 kB)
signature.asc (499.00 B)
Download all attachments

2018-12-13 08:53:52

[permalink] [raw]

Subject: Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > Implement the get_state() function and set the initial state to reflect
> > real state of the hardware. This allows to keep the PWM running if it was
> > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > set as output in bootloader keep the same setting in Linux unless it is
> > reconfigured.
> >
> > If we find the PWM block enabled we need to prepare and enable its source
> > clock otherwise the clock will be disabled late in the boot as unused.
> > That will leave the PWM in enabled state but with disabled clock. That has
> > a side effect that the PWM output is left at its current level at which
> > the clock was disabled. It is totally non-deterministic and it may be LOW
> > or HIGH.
> >
> > Signed-off-by: Michal Vokáč <[email protected]>
> > ---
> > drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 53 insertions(+)
>
> Applied, thanks.

Did you miss my feedback for this patch or did you choose to ignore it?

Best regards
Uwe

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

2018-12-13 17:02:51

[permalink] [raw]

Subject: Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-König wrote:
> On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > > Implement the get_state() function and set the initial state to reflect
> > > real state of the hardware. This allows to keep the PWM running if it was
> > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > set as output in bootloader keep the same setting in Linux unless it is
> > > reconfigured.
> > >
> > > If we find the PWM block enabled we need to prepare and enable its source
> > > clock otherwise the clock will be disabled late in the boot as unused.
> > > That will leave the PWM in enabled state but with disabled clock. That has
> > > a side effect that the PWM output is left at its current level at which
> > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > or HIGH.
> > >
> > > Signed-off-by: Michal Vokáč <[email protected]>
> > > ---
> > > drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 53 insertions(+)
> >
> > Applied, thanks.
>
> Did you miss my feedback for this patch or did you choose to ignore it?

Don't have anything in my inbox, but I see that it's there on patchwork,
so I suspect it was eaten by the spam filter. Let me copy-paste here and
go through it.

> > @@ -93,6 +96,55 @@ struct imx_chip {
> >
> > #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> >
> > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm, struct pwm_state *state)
>
> broken alignment.

I can fix that up, no need to resend for that.

> > +{
> > + struct imx_chip *imx = to_imx_chip(chip);
> > + u32 period, prescaler, pwm_clk, ret, val;
> > + u64 tmp;
> > +
> > + val = readl(imx->mmio_base + MX3_PWMCR);
> > +
> > + if (val & MX3_PWMCR_EN) {
> > + state->enabled = true;
> > + ret = clk_prepare_enable(imx->clk_per);
> > + if (ret)
> > + return;
> > + } else {
> > + state->enabled = false;
> > + }
> > +
> > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > + case MX3_PWMCR_POUTC_NORMAL:
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + break;
> > + case MX3_PWMCR_POUTC_INVERTED:
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + break;
> > + default:
> > + dev_warn(chip->dev, "can't set polarity, output disconnected");
>
> Should we return an error here?

We can't return an error here because the function has a void return
type. I'm not sure what it means if the POUTC is neither "normal" nor
"inverted", but perhaps a good idea would be to default to either of
those two in that case, or perhaps forcibly disable the PWM so that
we get known-good values in the registers?

I'm tempted not to treat this as a blocker because it's not actually
a bug or anything. Prior to this patch we also ignore whatever this
field was set to.

> > + }
> > +
> > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > + pwm_clk = clk_get_rate(imx->clk_per);
> > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > + val = readl(imx->mmio_base + MX3_PWMPR);
>
> It would be more cautionous to not rely on the reserved bits to read as
> zero. So I suggest to mask the value with 0xffff.

If that's what the documentation says I think it's okay to rely on it.
But I can add the mask if we want to play it extra safe.

> > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > +
> > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
>
> Would it make sense to introduce a policy about how to round in this
> case? (Similarily for .apply?) This is of course out of scope for this
> patch.

I'm not sure what you means by policy. The above already does round to
closest. Is that not an appropriate policy?

Thierry

Attachments:

(No filename) (3.93 kB)
signature.asc (849.00 B)
Download all attachments

2018-12-13 20:17:13

[permalink] [raw]

Subject: Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout

Hello Thierry,

On Thu, Dec 13, 2018 at 06:00:00PM +0100, Thierry Reding wrote:
> On Thu, Dec 13, 2018 at 09:52:13AM +0100, Uwe Kleine-König wrote:
> > On Wed, Dec 12, 2018 at 11:54:32AM +0100, Thierry Reding wrote:
> > > On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
> > > > Implement the get_state() function and set the initial state to reflect
> > > > real state of the hardware. This allows to keep the PWM running if it was
> > > > enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> > > > set as output in bootloader keep the same setting in Linux unless it is
> > > > reconfigured.
> > > >
> > > > If we find the PWM block enabled we need to prepare and enable its source
> > > > clock otherwise the clock will be disabled late in the boot as unused.
> > > > That will leave the PWM in enabled state but with disabled clock. That has
> > > > a side effect that the PWM output is left at its current level at which
> > > > the clock was disabled. It is totally non-deterministic and it may be LOW
> > > > or HIGH.
> > > >
> > > > Signed-off-by: Michal Vokáč <[email protected]>
> > > > ---
> > > > drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 53 insertions(+)
> > >
> > > Applied, thanks.
> >
> > Did you miss my feedback for this patch or did you choose to ignore it?
>
> Don't have anything in my inbox, but I see that it's there on patchwork,
> so I suspect it was eaten by the spam filter.

:-|

> Let me copy-paste here and go through it.
>
> > > @@ -93,6 +96,55 @@ struct imx_chip {
> > >
> > > #define to_imx_chip(chip) container_of(chip, struct imx_chip, chip)
> > >
> > > +static void imx_pwm_get_state(struct pwm_chip *chip,
> > > + struct pwm_device *pwm, struct pwm_state *state)
> >
> > broken alignment.
>
> I can fix that up, no need to resend for that.

fine.

> > > +{
> > > + struct imx_chip *imx = to_imx_chip(chip);
> > > + u32 period, prescaler, pwm_clk, ret, val;
> > > + u64 tmp;
> > > +
> > > + val = readl(imx->mmio_base + MX3_PWMCR);
> > > +
> > > + if (val & MX3_PWMCR_EN) {
> > > + state->enabled = true;
> > > + ret = clk_prepare_enable(imx->clk_per);
> > > + if (ret)
> > > + return;
> > > + } else {
> > > + state->enabled = false;
> > > + }
> > > +
> > > + switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> > > + case MX3_PWMCR_POUTC_NORMAL:
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > + break;
> > > + case MX3_PWMCR_POUTC_INVERTED:
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + break;
> > > + default:
> > > + dev_warn(chip->dev, "can't set polarity, output disconnected");
> >
> > Should we return an error here?
>
> We can't return an error here because the function has a void return
> type. I'm not sure what it means if the POUTC is neither "normal" nor
> "inverted", but perhaps a good idea would be to default to either of
> those two in that case, or perhaps forcibly disable the PWM so that
> we get known-good values in the registers?

The manual says "PWM output is disconnected". So I think the right thing
to do is to consider the pwm as disabled. (But checking how the output
actually behaves would be great.) (Maybe this setting is even the
solution to our "Make the output high-Z on disable" discussion?)

> I'm tempted not to treat this as a blocker because it's not actually
> a bug or anything. Prior to this patch we also ignore whatever this
> field was set to.

Prior to this patch this field was never read as this is only needed in
.get_state. And in .apply the value is correctly written.

> > > + }
> > > +
> > > + prescaler = MX3_PWMCR_PRESCALER_GET(val);
> > > + pwm_clk = clk_get_rate(imx->clk_per);
> > > + pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> > > + val = readl(imx->mmio_base + MX3_PWMPR);
> >
> > It would be more cautionous to not rely on the reserved bits to read as
> > zero. So I suggest to mask the value with 0xffff.
>
> If that's what the documentation says I think it's okay to rely on it.
> But I can add the mask if we want to play it extra safe.

The PERIOD field only covers bits 0 to 15 and the upper 16 bits of this
register are reserved. The registers are documented to read as 0, still
I'd prefer to mask them out for maximal correctness.

> > > + period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> > > +
> > > + /* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> > > + tmp = NSEC_PER_SEC * (u64)(period + 2);
> > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> >
> > Would it make sense to introduce a policy about how to round in this
> > case? (Similarily for .apply?) This is of course out of scope for this
> > patch.
>
> I'm not sure what you means by policy. The above already does round to
> closest. Is that not an appropriate policy?

I mean to document the expectation of the pwm framework how the driver
is supposed to configure the hardware when it should program the PWM
with say duty_cycle=100ns/period=300ns and but the hardware can only
implement both duty_cycle and period in steps of 3ns. So the (IMHO) good
candidates are:

duty_cycle = 99ns / period = 297ns
duty_cycle = 99ns / period = 300ns
duty_cycle = 102ns / period = 300ns
duty_cycle = 102ns / period = 306ns

As it's not obvious which is the best here (or is it?) and taking into
account that the PWM user might care, it would be right to specify
something like:

The driver is supposed to configure the biggest duty_cycle that
is not greater than the requested duty_cycle. Similarily it should
pick the biggest possible period that is not greater than the
requested period.

To actually allow the user to find the setting that he or she prefers
something like the clk framework provides with clk_round_rate() would be
needed of course.

And for .get_state I'd specify something similar that ensures that in
the sequence

myops->get_state(chip, pwm, state);
myops->apply(chip, pwm, state);

the latter is a noop.

Best regards
Uwe

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

2018-12-14 16:43:26

[permalink] [raw]

Subject: Re: [3/3] pwm: imx: Implement get_state() function for hardware readout

On 12.12.2018 11:51, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal Vokáč wrote:
>> Implement the get_state() function and set the initial state to reflect
>> real state of the hardware. This allows to keep the PWM running if it was
>> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
>> set as output in bootloader keep the same setting in Linux unless it is
>> reconfigured.
>>
>> If we find the PWM block enabled we need to prepare and enable its source
>> clock otherwise the clock will be disabled late in the boot as unused.
>> That will leave the PWM in enabled state but with disabled clock. That has
>> a side effect that the PWM output is left at its current level at which
>> the clock was disabled. It is totally non-deterministic and it may be LOW
>> or HIGH.
>
> Does this problem still exist if the pwm-imx driver is a module?

Yes. The source clock for PWM is stopped by the clock core shortly
before init process is started and it does not matter if the pwm-imx
driver is built as a module or built into the kernel.

When the module is loaded, the .get_state function is executed and
reads the PWM registers. If the EN bit is set the clock is started
and PWM continues from where it was stopped.

Michal

2018-12-14 16:59:26