Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx] > Sent: 2019年3月20日 18:58 > To: Anson Huang <anson.huang@xxxxxxx> > Cc: thierry.reding@xxxxxxxxx; 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>; > schnitzeltony@xxxxxxxxx; Robin Gong <yibin.gong@xxxxxxx>; linux- > pwm@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; dl-linux-imx > <linux-imx@xxxxxxx> > Subject: Re: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support > > Hello Anson, > > On Wed, Mar 20, 2019 at 10:17:50AM +0000, Anson Huang wrote: > > > On Wed, Mar 20, 2019 at 05:06:21AM +0000, Anson Huang wrote: > > > > + /* make sure counter is disabled for programming prescale */ > > > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > > > + saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val); > > > > + if (saved_cmod) { > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > > > I thought we agreed on not stopping the counter if the PS field isn't > changed? > > > > If the PS field no need to change, the round state should already > > return the period equal to current period settings, so this function will NOT > be called, right? > > > > if (real_state.period != tpm->real_period) { > > if (tpm->user_count > 1) { > > ret = -EBUSY; > > goto exit; > > } > > > > pwm_imx_tpm_setup_period(chip, param); > > tpm->real_period = real_state.period; > > } > > If the PS field is already right .period might still not match as its value > depends on SC.PS and MOD.MOD. Ah, my bad... I did NOT know what I was thinking... Yes, I will add the PS check to decide whether disabling counter.. > > > > > + 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: according to RM, the MOD register is > > > > + * updated immediately after CMOD[1:0] = 2b'00 above > > > > + */ > > > > > > So the current period isn't completed? That's wrong. > > > > So you mean we have to wait for the current period to finish here? > > I did NOT know this, so we have to compare the counter value with > > Yeah, see > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch > work.ozlabs.org%2Fpatch%2F1021566%2F&data=02%7C01%7Canson.h > uang%40nxp.com%7C626a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc > 2b4c6fa92cd99c5c301635%7C0%7C0%7C636886762757876916&sdata=r > wu%2BUo3GlRX8j4lXSOVuAs7n1yEuP5P2W6vhY%2BjiXdQ%3D&reserve > d=0 which documents this but waits for feedback by Thierry since some time. > > > the MOD value until they match then proceed the period change? > > If the hardware doesn't support you here (usually it does) I think it's not > reliable enough to try to sync in software. I assume you can get the right > wave form if you don't change SC.PS but only MOD.MOD? So maybe the > sanest approach is to refuse changing SC.PS if the PWM is running. > > Or disable (which also should wait for the current period to finish), poll for > the end (or use an irq?) and then setup the new configuration. Let me try to poll the TOF (timer overflow) before setup the new configuration. And will also need to add timeout for the polling, what shoud the timeout value be, 100ms? As ideally the max period can be very large, several seconds or even large, so is the 100mS good here? > > > > > +{ > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + struct pwm_state c; > > > > + u32 val, sc_val; > > > > + u64 tmp; > > > > + > > > > + pwm_imx_tpm_get_state(chip, pwm, &c); > > > > + > > > > + if (state.duty_cycle != c.duty_cycle) { > > > > + /* set duty counter */ > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > PWM_IMX_TPM_MOD_MOD; > > > > + tmp *= state.duty_cycle; > > > > + val = DIV_ROUND_CLOSEST_ULL(tmp, state.period); > > > > + writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm- > >hwpwm)); > > > > + } > > > > + > > > > + if (state.enabled != c.enabled) { > > > > > > This is wrong. If the PWM was running (c.enabled == true) and you > > > are supposed to disable (state.enabled == false) you enable the > > > hardware once more. > > > > A little confused here, as the case you assume, inside this block, > > there is another check for state.enabled, if it is false, the polarity > > will be set to channel disabled mode, the polarity setting is combined > together with the enable status here, am I wrong? > > Ah, you're right. I missed that probably because I saw register accesses after > the state.enabled != c.enabled check. > > > > > + val |= (state.polarity == PWM_POLARITY_NORMAL) ? > > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x2) : > > > > + FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0x1); > > > > > > Introduce PWM_IMX_TPM_CnSC_ELS_POLARITY_NORMAL and use it > together > > > with PWM_IMX_TPM_CnSC_ELS_POLARITY_INVERSED here. If you put > the > > > FIELD_PREP into the definition the line doesn't get excessively long. > > > > > > > I put the FIELD_PREP into definition, the line still long, but I agree using > definition is better. > > > > #define PWM_IMX_TPM_CnSC_ELS_INVERSED > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 1) > > #define PWM_IMX_TPM_CnSC_ELS_NORMAL > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 2) > > > > val |= (state->polarity == PWM_POLARITY_NORMAL) ? > > PWM_IMX_TPM_CnSC_ELS_NORMAL : > > PWM_IMX_TPM_CnSC_ELS_INVERSED; > > > > > Maybe also add > > > > > > #define PWM_IMX_TPM_CnSC_ELS_INACTIVE > > > FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) > > > > > > > I did NOT use the FIELD_PREP(PWM_IMX_TPM_CnSC_ELS, 0) at all, so why > add it? > > I don't quite understand. > > You use it implicitly in pwm_imx_tpm_apply_hw() if state.enabled == false > and c.enabled == true: > > val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > val &= ~(PWM_IMX_TPM_CnSC_ELS | ...); > ... > writel(val, tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); Ah, OK, I can replace the register field clear with the field prepare definition. > > > > > +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 imx_tpm_pwm_param param; > > > > + struct pwm_state real_state; > > > > + int ret; > > > > + > > > > + ret = pwm_imx_tpm_round_state(chip, ¶m, state, &real_state); > > > > + if (ret) > > > > + return -EINVAL; > > > > + > > > > + mutex_lock(&tpm->lock); > > > > + > > > > + /* > > > > + * 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 (real_state.period != tpm->real_period) { > > > > + if (tpm->user_count > 1) { > > > > + ret = -EBUSY; > > > > + goto exit; > > > > + } > > > > + > > > > + pwm_imx_tpm_config_counter(chip, param); > > > > + tpm->real_period = real_state.period; > > > > + } > > > > > > Maybe add a comment that this could still be optimized. For example > > > if pwm_imx_tpm_round_state returned prescale = 5 but prescale is > > > currently 6, you might still be able to configure > > > > You meant for multiple users request different period case? In this > > block, if there is ONLY one user and the requested period can be met > > by HW, it will anyway re-configure the HW for the prescale and period I > think, or I did NOT get your point? > > My idea has a flaw. I thought that if there is another user, the duty_cycle can > still be represented if the actually used prescale value is slightly higher. But > then there is still a problem with the period length that I missed. So my > remark was wrong, sorry for that. Thanks, Anson > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C62 > 6a6f5603f74ae0d37e08d6ad22e774%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636886762757876916&sdata=JsNRa8DuGYizE7FCyHVuY > QSUu4eUu5qTh6Edpf3Azm8%3D&reserved=0 |
