Hi, Thierry Best Regards! Anson Huang > -----Original Message----- > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > Sent: 2019年3月18日 23:31 > To: Anson Huang <anson.huang@xxxxxxx> > Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > linux@xxxxxxxxxxxxxxx; otavio@xxxxxxxxxxxxxxxx; stefan@xxxxxxxx; > Leonard Crestez <leonard.crestez@xxxxxxx>; Robin Gong > <yibin.gong@xxxxxxx>; jan.tuerk@xxxxxxxxxxx; linux- > pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; u.kleine- > koenig@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support > > On Mon, Mar 18, 2019 at 11:33:33AM +0000, Anson Huang wrote: > > Hi, Thierry > > > > Best Regards! > > Anson Huang > > > > > -----Original Message----- > > > From: Thierry Reding [mailto:thierry.reding@xxxxxxxxx] > > > Sent: 2019年3月18日 18:28 > > > To: Anson Huang <anson.huang@xxxxxxx> > > > Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; > > > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > > > linux@xxxxxxxxxxxxxxx; otavio@xxxxxxxxxxxxxxxx; stefan@xxxxxxxx; > > > Leonard Crestez <leonard.crestez@xxxxxxx>; Robin Gong > > > <yibin.gong@xxxxxxx>; jan.tuerk@xxxxxxxxxxx; linux- > > > pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm- > > > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; u.kleine- > > > koenig@xxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > > > Subject: Re: [PATCH V5 2/5] pwm: Add i.MX TPM PWM driver support > > > > > > On Mon, Mar 18, 2019 at 07:41:42AM +0000, Anson Huang wrote: > [...] > > > > +static void pwm_imx_tpm_config(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + u32 period, > > > > + u32 duty_cycle, > > > > + enum pwm_polarity polarity) { > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + u32 duty_cnt, val; > > > > + u64 tmp; > > > > + > > > > + /* set duty counter */ > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > > > PWM_IMX_TPM_MOD_MOD; > > > > + tmp *= duty_cycle; > > > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period); > > > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD, > > > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > > + > > > > + /* > > > > + * set polarity (for edge-aligned PWM modes) > > > > + * > > > > + * CPWMS MSB:MSA ELSB:ELSA Mode Configuration > > > > + * 0 10 10 PWM High-true pulse > > > > + * 0 10 00 PWM Reserved > > > > + * 0 10 01 PWM Low-true pulse > > > > + * 0 10 11 PWM Reserved > > > > + * > > > > + * High-true pulse: clear output on counter match, set output on > > > > + * counter reload, set output when counter first enabled or paused. > > > > + * > > > > + * Low-true pulse: set output on counter match, clear output on > > > > + * counter reload, clear output when counter first enabled or paused. > > > > + */ > > > > + > > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > + val &= ~(PWM_IMX_TPM_CnSC_ELSB | PWM_IMX_TPM_CnSC_ELSA > > > | > > > > + PWM_IMX_TPM_CnSC_MSA); > > > > + val |= PWM_IMX_TPM_CnSC_MSB; > > > > + val |= (polarity == PWM_POLARITY_NORMAL) ? > > > > + PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA; > > > > + /* > > > > + * polarity settings will enabled/disable output status > > > > + * immediately, so here ONLY save the config and write > > > > + * it into register when channel is enabled/disabled. > > > > + */ > > > > + tpm->chn_config[pwm->hwpwm] = val; } > > > > + > > > > +/* > > > > + * When a channel's polarity is configured, the polarity settings > > > > + * will be saved and ONLY write into the register when the > > > > +channel > > > > + * is enabled. > > > > + * > > > > + * When a channel is disabled, its polarity settings will be > > > > +saved > > > > + * and its output will be disabled by clearing polarity settings. > > > > + * > > > > + * when a channel is enabled, its polarity settings will be > > > > +restored > > > > > > "when" -> "When". > > > > Will fix it. > > > > > > > > > + * and output will be enabled again. > > > > + */ > > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + bool enable) > > > > +{ > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + u32 val; > > > > + > > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > > + if (enable) { > > > > + /* restore channel config */ > > > > + writel(tpm->chn_config[pwm->hwpwm], > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > + > > > > + if (++tpm->enable_count == 1) { > > > > + /* start TPM counter */ > > > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > + } > > > > + } else { > > > > + /* disable channel */ > > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm- > > > >hwpwm)); > > > > + val &= ~(PWM_IMX_TPM_CnSC_MSA | > > > PWM_IMX_TPM_CnSC_MSB | > > > > + PWM_IMX_TPM_CnSC_ELSB | > > > PWM_IMX_TPM_CnSC_ELSA); > > > > + writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm- > > > >hwpwm)); > > > > + > > > > + if (--tpm->enable_count == 0) { > > > > + /* stop TPM counter since all channels are disabled > > > */ > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > + } > > > > + } > > > > + > > > > + /* update channel status */ > > > > + tpm->chn_status[pwm->hwpwm] = enable; } > > > > + > > > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip, > > > > + struct pwm_device *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + u64 tmp; > > > > + u32 val, rate; > > > > + > > > > + /* get period */ > > > > + rate = clk_get_rate(tpm->clk); > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD); > > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > > + val &= PWM_IMX_TPM_SC_PS; > > > > + tmp *= (1 << val) * NSEC_PER_SEC; > > > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > > > + > > > > + /* get duty cycle */ > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > > > + tmp *= (1 << val) * NSEC_PER_SEC; > > > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > > > + > > > > + /* get polarity */ > > > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > + if (val & PWM_IMX_TPM_CnSC_ELSA) > > > > + state->polarity = PWM_POLARITY_INVERSED; > > > > + else > > > > + state->polarity = PWM_POLARITY_NORMAL; > > > > + > > > > + /* get channel status */ > > > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; } > > > > + > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct > > > > +pwm_device > > > *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + struct pwm_state curstate; > > > > + int ret; > > > > + > > > > + mutex_lock(&tpm->lock); > > > > + > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > > + > > > > + if (state->period != curstate.period) { > > > > + /* > > > > + * TPM counter is shared by multiple channels, so > > > > + * prescale and period can NOT be modified when > > > > + * there are multiple channels in use. > > > > + */ > > > > + if (tpm->user_count != 1) > > > > + return -EBUSY; > > > > + ret = pwm_imx_tpm_config_counter(chip, state->period); > > > > + if (ret) > > > > + return ret; > > > > + } > > > > + > > > > + if (state->enabled == false) { > > > > + /* > > > > + * if eventually the PWM output is LOW, either > > > > + * duty cycle is 0 or status is disabled, need > > > > + * to make sure the output pin is LOW. > > > > + */ > > > > + pwm_imx_tpm_config(chip, pwm, state->period, > > > > + 0, state->polarity); > > > > + if (curstate.enabled) > > > > + pwm_imx_tpm_enable(chip, pwm, false); > > > > + } else { > > > > + pwm_imx_tpm_config(chip, pwm, state->period, > > > > + state->duty_cycle, state->polarity); > > > > + if (!curstate.enabled) > > > > + pwm_imx_tpm_enable(chip, pwm, true); > > > > > > Doesn't this mean that you won't be applying changes to the polarity > > > while a PWM is enabled? That seems wrong. Granted, you may usually > > > not run into that, but if you can't support it I think you should at > > > least return an error if you detect that the user wants to change polarity > while the PWM is enabled. > > > > I thought below function call already set the polarity? No matter its > > status is enabled or disabled, the polarity setting will be always applied. > > > > pwm_imx_tpm_config(chip, pwm, state->period, > > state->duty_cycle, state->polarity); > > That's not what it seems to do. In fact there's a comment that explains why it > doesn't do that. Quoting here: > > > > > + /* > > > > + * polarity settings will enabled/disable output status > > > > + * immediately, so here ONLY save the config and write > > > > + * it into register when channel is enabled/disabled. > > > > + */ > > > > + tpm->chn_config[pwm->hwpwm] = val; > > Looks to me like that only stores the value for that register so that it can be > applied at a later point. Or am I missing something? The comment is what I intend to say, since the hardware does NOT provide the channel enable/disable function, but different polarity settings can act as channel enable/disable, that is why I store the polarity setting and ONLY apply it to the hardware when the channel is enabled. > > > > > + ret); > > > > + } > > > > + > > > > + return ret; > > > > +}; > > > > > > Your handling of the clock seems strange here. Basically in the > > > above you always keep the clock on and you only disable it if there > > > are no users and you're going to suspend. > > > > > > Why do you need to keep an extra reference count anyway? Or why keep > > > the clock on all the time? Can't you just do a clk_prepare_enable() > > > every time somebody enables the PWM? The CCF already has built-in > > > reference counting, so I'm not sure you really need to duplicate that here. > > > > Keeping clock always ON since driver probe is because, many TMP registers' > > write needs clock to be ON for sync into register hardware, just > > enable the clock before writing register and disable the clock > > immediately does NOT work, unless we keep reading the register value > > until the register value is what we want to write, but that makes code > > much more complicated, and the PWM clock normally is from OSC which > > does NOT consume too much power, so I keep the clock always on and > ONLY disable it after suspend. > > Why do you bother with keeping the enable reference count, then? Can't you > just enable the clock on probe, then on suspend disable it, enable it again on > resume and on remove you also disable it? Why does it need to be > dependent on whether there are any active PWMs or not? That was what I did in previous patch version, but I received comments saying that PWM clock should NOT be disabled if it is in use, and the channel enable count is introduced for determine whether to disable the shared HW counter, so I also use the channel enable count to determine whether to turn off clock in suspend. Although I think it should be OK to just turn off clock in suspend without any checking, if there is any active PWM, then the user should fix it, as I think PWM should be OFF for suspend. So you also think I can skip the check for the PWM active count and just disable clock in suspend? Thanks, Anson. > > Thierry
