Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx] > Sent: 2019年3月21日 21:42 > 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; stefan@xxxxxxxx; > otavio@xxxxxxxxxxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>; > schnitzeltony@xxxxxxxxx; jan.tuerk@xxxxxxxxxxx; 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 V8 2/5] pwm: Add i.MX TPM PWM driver support > > Hello, > > On Thu, Mar 21, 2019 at 12:47:32PM +0000, Anson Huang wrote: > > > On Thu, Mar 21, 2019 at 09:54:15AM +0000, Anson Huang wrote: > > > > > On Thu, Mar 21, 2019 at 12:47:57AM +0000, Anson Huang wrote: > > > > > > +static void pwm_imx_tpm_setup_period(struct pwm_chip *chip, > > > > > > + struct imx_tpm_pwm_param *p) { > > > > > > + struct imx_tpm_pwm_chip *tpm = > to_imx_tpm_pwm_chip(chip); > > > > > > + u32 val, saved_cmod, cur_prescale; > > > > > > + > > > > > > + /* make sure counter is disabled for programming prescale > */ > > > > > > > > > > @Thierry: What is your thought here? IMHO this should only be > > > > > allowed if all affected PWMs are off. > > > > > > > > As we already make sure that ONLY when there is ONLY 1 user and > > > > the requested period is different from the current period, then > > > > this function will be called, so there is impossible that multiple > > > > PWMs are active > > > and the period is requested to be changed. > > > > Am I right? > > > > > > This problem is not about two PWMs. If you reconfigure a running PWM > > > the requirement is that the hardware completes a whole period with > > > the old configuration and then immediately starts a new period with > > > the new parameters. If you stop the counter, the last period with > > > the old parameters is disturbed. > > > > So, I think simply return error if the counter is running and there is > > a new PS change request, right? > > That's the general idea yes. If you cannot fulfil the request without violating > the guarantees you have to adhere, refuse the request with an error. Already add below check in next version: 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; > > > > > > > + */ > > > > > > + state->polarity = PWM_POLARITY_NORMAL; > > > > > > + > > > > > > + /* get channel status */ > > > > > > + state->enabled = FIELD_GET(PWM_IMX_TPM_CnSC_ELS, val) ? > true : > > > > > > +false; } > > > > > > + > > > > > > +static void pwm_imx_tpm_apply_hw(struct pwm_chip *chip, > > > > > > + struct pwm_device *pwm, > > > > > > + struct pwm_state *state) > > > > > > > > > > pwm_imx_tpm_apply_hw is called with the mutex hold. Is this > necessary? > > > > > Please either call it without the mutex or annotate the function > > > > > that the caller is supposed to hold it. > > > > > > > > OK, will make sure call it without mutex hold. After further check, since this function will call get_state and it will access shared registers, so mutex is necessary I think, so I will add annotate for this function. > > Reading through the reference manual I noticed that there might be a > stall: If you write two values to CnV the second write is ignored if the first > wasn't latched yet. That might mean that you cannot release the mutex > before the newly configured state is active. This is related to the request to > not let .apply return before the configured state is active, but I didn't thought > this to an end what the real consequences have to be. The reference manual says the register is NOT updated until the current period finished If counter is running, so I added below check for both period update and duty update, we Can just wait the register value read matches what we write: Period update: writel(p->mod, tpm->base + PWM_IMX_TPM_MOD); /* make sure MOD register is updated */ timeout = jiffies + msecs_to_jiffies(tpm->real_period / NSEC_PER_MSEC + 1); while (readl(tpm->base + PWM_IMX_TPM_MOD != p->mod)) { if (time_after(jiffies, timeout)) return = -ETIME; cpu_relax(); } Duty update: writel(val, tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); /* make sure CnV register is updated */ timeout = jiffies + msecs_to_jiffies(tpm->real_period / NSEC_PER_MSEC + 1); while (readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)) != val) { if (time_after(jiffies, timeout)) return = -ETIME; cpu_relax(); } > > > > > If counter is enabled, and for edge aligned PWM mode(EPWM), the > > > > register is updated After written and TPM counter changes from MOD > > > > to zero, same as period count update, HW will make sure the period > finish.. > > > > > > Looking into my concern again, it is actually the other way around: > > > Assuming a single used PWM channel that runs at duty_cycle=500 + > > > period=1000. Then pwm_imx_tpm_apply() is called with state- > > > >duty_cycle=700 and state->period=800. pwm_imx_tpm_apply() calls > > > pwm_imx_tpm_setup_period() to configure for .period=1000. Now if the > > > PWM completes a period before pwm_imx_tpm_apply_hw() sets up CnV > to > > > the value corresponding to duty_cycle=700, it produces a waveform > > > with > > > duty_cycle=500 and period=800 which is bad. This is another > > > limitation that can be worked around in software with some effort > > > (which might or might not be worth to spend). > > > > I am sure that on i.MX7ULP platform we used for backlight ONLY, it > > should NOT that matter if this case happen, unless the counter is > > disabled, then the effort spend on this case will be huge, so I plan to leave > it as what it is if you don't mind. > > That means you have to add this to the list of limitations. OK, will try best to describe this scenario in limitation note; > > > > > > > +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_setup_period(chip, ¶m); > > > > > > + tpm->real_period = real_state.period; > > > > > > + } > > > > > > + > > > > > > + pwm_imx_tpm_apply_hw(chip, pwm, &real_state); > > > > > > + > > > > > > +exit: > > > > > > + mutex_unlock(&tpm->lock); > > > > > > > > > > .apply is supposed to sleep until the newly configured state is active. > > > > > This is missing here, right? > > > > > > > > NOT quite understand, you meant .apply could be sleep if mutex is > > > > hold by other thread? > > > > > > No. .apply is supposed to only return when the new configuration is > active. > > > So if the PWM is running in its previous configuration, you setup > > > the registers such that the new configuration gets active in the > > > next period, you must not yet return to the caller until the new period > started. > > > > That bring me back to previous question, we can add waiting for period > > finish And then return from .apply, but we also need a timeout for the > > wait, what should The timeout value be? 100mS? Or even several seconds? > > A sane value would be the duration of the previously configured period > length as this is the theoretical upper limit for this delay. I will just use tpm->real_period saved in driver data, for first time update, although Tpm->real_period is 0, but the counter is disabled, so the register update is immediately, >From second time, the tpm->real_period will be previous period configured. 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%7Cae > 5949f109c5401a2b5d08d6ae030d7f%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C636887725468509805&sdata=4qU1eJzlIle9v%2BHqEqwb > Jm%2FkkAQlCH2LtsFDMaeSGHE%3D&reserved=0 |
