* [PATCH 1/3] pwm: imx: Sort include files @ 2018-10-01 14:19 Michal Vokáč 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Michal Vokáč @ 2018-10-01 14:19 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-pwm, linux-kernel, Michal Vokáč Sort included header files alphabetically. Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com> --- drivers/pwm/pwm-imx.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c index 1d5242c..bcbcac4 100644 --- a/drivers/pwm/pwm-imx.c +++ b/drivers/pwm/pwm-imx.c @@ -5,17 +5,17 @@ * Derived from pxa PWM driver by eric miao <eric.miao@marvell.com> */ -#include <linux/module.h> -#include <linux/kernel.h> -#include <linux/platform_device.h> -#include <linux/slab.h> -#include <linux/err.h> #include <linux/clk.h> #include <linux/delay.h> +#include <linux/err.h> #include <linux/io.h> -#include <linux/pwm.h> +#include <linux/kernel.h> +#include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> /* i.MX1 and i.MX21 share the same PWM function block: */ -- 2.1.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values 2018-10-01 14:19 [PATCH 1/3] pwm: imx: Sort include files Michal Vokáč @ 2018-10-01 14:19 ` Michal Vokáč 2018-12-12 10:53 ` Thierry Reding 2018-12-12 10:55 ` [2/3] " Uwe Kleine-König 2018-10-01 14:19 ` [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout Michal Vokáč 2018-12-12 10:52 ` [PATCH 1/3] pwm: imx: Sort include files Thierry Reding 2 siblings, 2 replies; 13+ messages in thread From: Michal Vokáč @ 2018-10-01 14:19 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-pwm, linux-kernel, Michal Vokáč 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áč <michal.vokac@ysoft.com> --- 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 <eric.miao@marvell.com> */ +#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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč @ 2018-12-12 10:53 ` Thierry Reding 2018-12-12 10:55 ` [2/3] " Uwe Kleine-König 1 sibling, 0 replies; 13+ messages in thread From: Thierry Reding @ 2018-12-12 10:53 UTC (permalink / raw) To: Michal Vokáč; +Cc: linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 435 bytes --] 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áč <michal.vokac@ysoft.com> > --- > drivers/pwm/pwm-imx.c | 78 ++++++++++++++++++++++++++++++++++++++------------- > 1 file changed, 58 insertions(+), 20 deletions(-) Applied, thanks. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [2/3] pwm: imx: Use bitops and bitfield macros to define register values 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč 2018-12-12 10:53 ` Thierry Reding @ 2018-12-12 10:55 ` Uwe Kleine-König 1 sibling, 0 replies; 13+ messages in thread From: Uwe Kleine-König @ 2018-12-12 10:55 UTC (permalink / raw) To: Michal Vokáč; +Cc: Thierry Reding, linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5264 bytes --] 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áč <michal.vokac@ysoft.com> > --- > 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 <eric.miao@marvell.com> > */ > > +#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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-10-01 14:19 [PATCH 1/3] pwm: imx: Sort include files Michal Vokáč 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč @ 2018-10-01 14:19 ` Michal Vokáč 2018-12-12 10:51 ` [3/3] " Uwe Kleine-König 2018-12-12 10:54 ` [PATCH 3/3] " Thierry Reding 2018-12-12 10:52 ` [PATCH 1/3] pwm: imx: Sort include files Thierry Reding 2 siblings, 2 replies; 13+ messages in thread From: Michal Vokáč @ 2018-10-01 14:19 UTC (permalink / raw) To: Thierry Reding; +Cc: linux-pwm, linux-kernel, Michal Vokáč 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áč <michal.vokac@ysoft.com> --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [3/3] pwm: imx: Implement get_state() function for hardware readout 2018-10-01 14:19 ` [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout Michal Vokáč @ 2018-12-12 10:51 ` Uwe Kleine-König 2018-12-14 16:40 ` Vokáč Michal 2018-12-12 10:54 ` [PATCH 3/3] " Thierry Reding 1 sibling, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2018-12-12 10:51 UTC (permalink / raw) To: Michal Vokáč; +Cc: Thierry Reding, linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 3761 bytes --] 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áč <michal.vokac@ysoft.com> > --- > 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, > }; > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [3/3] pwm: imx: Implement get_state() function for hardware readout 2018-12-12 10:51 ` [3/3] " Uwe Kleine-König @ 2018-12-14 16:40 ` Vokáč Michal 0 siblings, 0 replies; 13+ messages in thread From: Vokáč Michal @ 2018-12-14 16:40 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Thierry Reding, linux-pwm, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-10-01 14:19 ` [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout Michal Vokáč 2018-12-12 10:51 ` [3/3] " Uwe Kleine-König @ 2018-12-12 10:54 ` Thierry Reding 2018-12-13 8:52 ` Uwe Kleine-König 1 sibling, 1 reply; 13+ messages in thread From: Thierry Reding @ 2018-12-12 10:54 UTC (permalink / raw) To: Michal Vokáč; +Cc: linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1002 bytes --] 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áč <michal.vokac@ysoft.com> > --- > drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) Applied, thanks. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-12-12 10:54 ` [PATCH 3/3] " Thierry Reding @ 2018-12-13 8:52 ` Uwe Kleine-König 2018-12-13 17:00 ` Thierry Reding 0 siblings, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2018-12-13 8:52 UTC (permalink / raw) To: Thierry Reding; +Cc: Michal Vokáč, linux-pwm, linux-kernel 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áč <michal.vokac@ysoft.com> > > --- > > 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/ | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-12-13 8:52 ` Uwe Kleine-König @ 2018-12-13 17:00 ` Thierry Reding 2018-12-13 20:14 ` Uwe Kleine-König 0 siblings, 1 reply; 13+ messages in thread From: Thierry Reding @ 2018-12-13 17:00 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: Michal Vokáč, linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 4027 bytes --] 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áč <michal.vokac@ysoft.com> > > > --- > > > 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-12-13 17:00 ` Thierry Reding @ 2018-12-13 20:14 ` Uwe Kleine-König 2018-12-14 16:57 ` Vokáč Michal 0 siblings, 1 reply; 13+ messages in thread From: Uwe Kleine-König @ 2018-12-13 20:14 UTC (permalink / raw) To: Thierry Reding; +Cc: Michal Vokáč, linux-pwm, linux-kernel, kernel 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áč <michal.vokac@ysoft.com> > > > > --- > > > > 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/ | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout 2018-12-13 20:14 ` Uwe Kleine-König @ 2018-12-14 16:57 ` Vokáč Michal 0 siblings, 0 replies; 13+ messages in thread From: Vokáč Michal @ 2018-12-14 16:57 UTC (permalink / raw) To: Uwe Kleine-König, Thierry Reding; +Cc: linux-pwm, linux-kernel, kernel On 13.12.2018 21:14, Uwe Kleine-König wrote:> 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áč <michal.vokac@ysoft.com> >>>>> --- >>>>> 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 checked the behavior. It can be used to disconnect the output even if the PWM is enabled. It does not solve the problem with the LOW output level in disabled state. This gave me the same results. When PWM is enabled and the output is disconnected with POUTC = 2, the output level is always LOW and the DSE, ODE, and PUS pad control settings have no effect. I suppose the main purpose of this is to allow to quickly enable/disable the output without the need to enable/ disable the whole PWM block. So we can not really say the PWM is enabled/disabled here as there is another (EN) bit in the PWMCR register that controls that. >> 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. OK. >>>> + 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. Sounds good. Would be great to find something like this in the docs while I was implementing the .get_state. If this is out of scope for this patch then there are only two minor issues to fix - the incorrect indentation and the mask for the period register, right? So given the fact that Thierry already applied the series I can send a separate patch with these fixes, OK? Or you will fix it up all yourself, Thierry? Best regards, Michal ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] pwm: imx: Sort include files 2018-10-01 14:19 [PATCH 1/3] pwm: imx: Sort include files Michal Vokáč 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč 2018-10-01 14:19 ` [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout Michal Vokáč @ 2018-12-12 10:52 ` Thierry Reding 2 siblings, 0 replies; 13+ messages in thread From: Thierry Reding @ 2018-12-12 10:52 UTC (permalink / raw) To: Michal Vokáč; +Cc: linux-pwm, linux-kernel [-- Attachment #1: Type: text/plain, Size: 307 bytes --] On Mon, Oct 01, 2018 at 04:19:46PM +0200, Michal Vokáč wrote: > Sort included header files alphabetically. > > Signed-off-by: Michal Vokáč <michal.vokac@ysoft.com> > --- > drivers/pwm/pwm-imx.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Applied, thanks. Thierry [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-12-14 16:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-01 14:19 [PATCH 1/3] pwm: imx: Sort include files Michal Vokáč 2018-10-01 14:19 ` [PATCH 2/3] pwm: imx: Use bitops and bitfield macros to define register values Michal Vokáč 2018-12-12 10:53 ` Thierry Reding 2018-12-12 10:55 ` [2/3] " Uwe Kleine-König 2018-10-01 14:19 ` [PATCH 3/3] pwm: imx: Implement get_state() function for hardware readout Michal Vokáč 2018-12-12 10:51 ` [3/3] " Uwe Kleine-König 2018-12-14 16:40 ` Vokáč Michal 2018-12-12 10:54 ` [PATCH 3/3] " Thierry Reding 2018-12-13 8:52 ` Uwe Kleine-König 2018-12-13 17:00 ` Thierry Reding 2018-12-13 20:14 ` Uwe Kleine-König 2018-12-14 16:57 ` Vokáč Michal 2018-12-12 10:52 ` [PATCH 1/3] pwm: imx: Sort include files Thierry Reding
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).