Hello,
On Tue, Apr 09, 2019 at 08:51:48AM +0000, Anson Huang wrote:
> > On Tue, Mar 26, 2019 at 06:52:33AM +0000, Anson Huang wrote:
> > > + /* get polarity */
> > > + if (chan) {
> > > + state->polarity = chan->polarity;
> > > + } else {
> > > + /* in case no channel requested yet, return HW status */
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + if (FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ==
> > > + PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED)
> > > + state->polarity = PWM_POLARITY_INVERSED;
> > > + else
> > > + /*
> > > + * Assume reserved values (2b00 and 2b11) to yield
> > > + * normal polarity.
> > > + */
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > + }
> >
> > What is the good reason to prefer chan->polarity over reading out the
> > hardware state?
>
> Reading it from DDR is faster than accessing HW register as per
> previous comment?
How much time do you save here? Is it worth to complicate the function
for that?
> > > + /* get channel status */
> > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? true :
> > > +false; }
> > > +
> > > +/* this function is supposed to be called with mutex hold */ static
> > > +int pwm_imx_tpm_apply_hw(struct pwm_chip *chip,
> > > + struct pwm_device *pwm,
> > > + struct pwm_state *state,
> > > + struct imx_tpm_pwm_param *p) {
> > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip);
> > > + struct imx_tpm_pwm_channel *chan = pwm_get_chip_data(pwm);
> > > + bool period_update = false;
> > > + bool duty_update = false;
> > > + u32 val, cmod, cur_prescale;
> > > + unsigned long timeout;
> > > + struct pwm_state c;
> > > +
> > > + if (state->period != tpm->real_period) {
> > > + /*
> > > + * TPM counter is shared by multiple channels, so
> > > + * prescale and period can NOT be modified when
> > > + * there are multiple channels in use with different
> > > + * period settings.
> > > + */
> > > + if (tpm->user_count > 1)
> > > + return -EBUSY;
> > > +
> > > + val = readl(tpm->base + PWM_IMX_TPM_SC);
> > > + cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val);
> > > + cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val);
> > > + if (cmod && cur_prescale != p->prescale)
> > > + return -EBUSY;
> > > +
> > > + /* set TPM counter prescale */
> > > + val &= ~PWM_IMX_TPM_SC_PS;
> > > + val |= FIELD_PREP(PWM_IMX_TPM_SC_PS, p->prescale);
> > > + writel(val, tpm->base + PWM_IMX_TPM_SC);
> > > +
> > > + /*
> > > + * set period count:
> > > + * if the PWM is disabled (CMOD[1:0] = 2b00), then MOD
> > > register
> > > + * is updated when MOD register is written.
> > > + *
> > > + * if the PWM is enabled (CMOD[1:0] ≠ 2b00), the period
> > > length
> > > + * is latched into hardware when the next period starts.
> > > + */
> > > + writel(p->mod, tpm->base + PWM_IMX_TPM_MOD);
> > > + tpm->real_period = state->period;
> > > + period_update = true;
> > > + }
> > > +
> > > + pwm_imx_tpm_get_state(chip, pwm, &c);
> >
> > If you move this call above the previous if block you can use c.period
> > instead
> > of tpm->real_period which is easier to follow.
>
> I think the period could be changed by the if block, so duty also be changed,
> need
> to put the .get_state here, am I right?
As you don't use c.period below this shouldn't matter. Where does duty
change?
> > > + if (state->duty_cycle != c.duty_cycle) {
> > > + /*
> > > + * set channel value:
> > > + * if the PWM is disabled (CMOD[1:0] = 2b00), then CnV
> > > register
> > > + * is updated when CnV register is written.
> > > + *
> > > + * if the PWM is enabled (CMOD[1:0] ≠ 2b00), the duty length
> > > + * is latched into hardware when the next period starts.
> > > + */
> > > + writel(p->val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm));
> > > + duty_update = true;
> > > + }
> > > +
> > > + /* make sure MOD & CnV registers are updated */
> > > + if (period_update || duty_update) {
> > > + timeout = jiffies + msecs_to_jiffies(tpm->real_period /
> > > + NSEC_PER_MSEC + 1);
> > > + while (readl(tpm->base + PWM_IMX_TPM_MOD) != p->mod
> > > + || readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm))
> > > + != p->val) {
> > > + if (time_after(jiffies, timeout))
> > > + return -ETIME;
> > > + cpu_relax();
> > > + }
> > > + }
> >
> > If the PWM is running you wait in the above loop until the new values are
> > active but before you configure the period. I think in the case where the
> > PWM is active and a change of polarity is requested it would be more correct
> > to refuse the change.
>
> Not very understand, the period is changed at the beginning, and most of the
> time,
> period should be fixed, changing polarity should be allowed even PWM is
> active?
Changing polarity should be atomic (that is, get active with the next
period's start). As the hardware doesn't support that, claiming it does
is a bad idea.
> That does NOT introduce too many trouble, is it a common case that dynamic
> changing
> polarity is NOT good?
>
>
> >
> > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm));
> > > + val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA |
> > > + PWM_IMX_TPM_CnSC_MSB);
> > > + if (state->enabled) {
> > > + /*
> > > + * set polarity (for edge-aligned PWM modes)
> > > + *
> > > + * ELS[1:0] = 2b10 yields normal polarity behaviour,
> > > + * ELS[1:0] = 2b01 yields inversed polarity.
> > > + * The other values are reserved.
> > > + *
> > > + * polarity settings will enabled/disable output status
> > > + * immediately, so if the channel is disabled, need to
> > > + * make sure MSA/MSB/ELS are set to 0 which means channel
> > > + * disabled.
> >
> > I don't understand this comment. Either ELS = 0 is reserved or it can be
> > used.
> > What is an output status?
>
> The reference manual ONLY states it as reserved, so how to add comments here?
The problem might just be, that I don't get what you intend to say in
the last paragraph.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
