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, > }; >
signature.asc
Description: PGP signature
