Hi, Uwe Best Regards! Anson Huang
> -----Original Message----- > From: Anson Huang > Sent: 2019年3月20日 19:21 > To: 'Uwe Kleine-König' <u.kleine-koe...@pengutronix.de> > Cc: thierry.red...@gmail.com; robh...@kernel.org; mark.rutl...@arm.com; > shawn...@kernel.org; s.ha...@pengutronix.de; ker...@pengutronix.de; > feste...@gmail.com; li...@armlinux.org.uk; ota...@ossystems.com.br; > ste...@agner.ch; Leonard Crestez <leonard.cres...@nxp.com>; > schnitzelt...@gmail.com; Robin Gong <yibin.g...@nxp.com>; linux- > p...@vger.kernel.org; devicet...@vger.kernel.org; linux-arm- > ker...@lists.infradead.org; linux-kernel@vger.kernel.org; dl-linux-imx > <linux-...@nxp.com> > Subject: RE: [PATCH V7 2/5] pwm: Add i.MX TPM PWM driver support > > Hi, Uwe > > Best Regards! > Anson Huang > > > -----Original Message----- > > From: Uwe Kleine-König [mailto:u.kleine-koe...@pengutronix.de] > > Sent: 2019年3月20日 18:58 > > To: Anson Huang <anson.hu...@nxp.com> > > Cc: thierry.red...@gmail.com; robh...@kernel.org; > > mark.rutl...@arm.com; shawn...@kernel.org; s.ha...@pengutronix.de; > > ker...@pengutronix.de; feste...@gmail.com; li...@armlinux.org.uk; > > ota...@ossystems.com.br; ste...@agner.ch; Leonard Crestez > > <leonard.cres...@nxp.com>; schnitzelt...@gmail.com; Robin Gong > > <yibin.g...@nxp.com>; linux- p...@vger.kernel.org; > > devicet...@vger.kernel.org; linux-arm- ker...@lists.infradead.org; > > linux-kernel@vger.kernel.org; dl-linux-imx <linux-...@nxp.com> > > 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.. I added below additional check for current PS and the new PS cur_prescale = FIELD_GET(PWM_IMX_TPM_SC_PS, val); if (saved_cmod && cur_prescale != p->prescale) { val &= ~PWM_IMX_TPM_SC_CMOD; writel(val, tpm->base + PWM_IMX_TPM_SC); } > > > > > > > > > + 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%2Fpatc > > h > 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? After further check, the reference manual has below statement, so I think we no need to care about it, the hardware make sure of that, I added below comment before programming the MOD register, if counter is disabled, the MOD register will be updated immediately, if counter is enabled, the CPWM bit is fixed as 0 in our driver, so the MOD register will be updated when counter changes from MOD to zero. /* * set period count: according to RM, the MOD register is * updated immediately if CMOD[1:0] = 2b'00. * if CMOD[1:0] != 2b'00, then MOD register is updated * according to the CPWMS bit, that is: * * If the selected mode is not CPWM then MOD register is * updated after MOD register was written and the TPM * counter changes from MOD to zero. * * If the selected mode is CPWM then MOD register is updated * after MOD register was written and the TPM counter changes * from MOD to (MOD – 1). */ > > > > > > > > +{ > > > > > + 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: But the place I used is just to clear the PWM_IMX_TPM_CnSC_ELS field, so just the MASK is enough for me, if you don't mind, I will leave it as what it is now. Anson. > > > > 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 |
