Best Regards! Anson Huang > -----Original Message----- > From: Anson Huang > Sent: 2019年4月9日 20:04 > To: 'Uwe Kleine-König' <u.kleine-koenig@xxxxxxxxxxxxxx> > Cc: mark.rutland@xxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; Robin Gong > <yibin.gong@xxxxxxx>; schnitzeltony@xxxxxxxxx; > otavio@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > festevam@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux@xxxxxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > thierry.reding@xxxxxxxxx; stefan@xxxxxxxx; kernel@xxxxxxxxxxxxxx; > Leonard Crestez <leonard.crestez@xxxxxxx>; shawnguo@xxxxxxxxxx; linux- > arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: RE: [EXT] Re: [PATCH V10 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-koenig@xxxxxxxxxxxxxx] > > Sent: 2019年4月9日 17:29 > > To: Anson Huang <anson.huang@xxxxxxx> > > Cc: mark.rutland@xxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; Robin Gong > > <yibin.gong@xxxxxxx>; schnitzeltony@xxxxxxxxx; > > otavio@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > festevam@xxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; linux@xxxxxxxxxxxxxxx; > > robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > thierry.reding@xxxxxxxxx; stefan@xxxxxxxx; kernel@xxxxxxxxxxxxxx; > > Leonard Crestez <leonard.crestez@xxxxxxx>; shawnguo@xxxxxxxxxx; > linux- > > arm-kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > > Subject: Re: [EXT] Re: [PATCH V10 2/5] pwm: Add i.MX TPM PWM driver > > support > > > > WARNING: This email was created outside of NXP. DO NOT CLICK links or > > attachments unless you recognize the sender and know the content is safe. > > > > > > > > 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? > > My intention is NOT to save much time here, it is just because that I > remembered there was comment before to suggest using variable stored in > DRAM instead of accessing HW register, so I am a little confused where > should use variable and where should access HW register. > > Also, variable can be used directly, while reading HW register will need to > translate the register field value to polarity. > > If it is better to read hardware state based on your experience, I will follow > the suggestion. Sorry for it, after looking into the code deeper, as there is already accessing HW register code in case of channel NOT requested, I think your suggestion makes more sense, I can remove all the channel private data to make code easy. Thanks, Anson > > > > > > > > + /* 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? > > The "prescale" is used during computing the duty, and it could be changed in > period 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. > > > > OK, that even makes driver easy, will change it in next version. > > > > > 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. > > For the configuration, MSA/MSB = 0/1 means edge-aligned PWM mode, in > this mode, ELS[1:0]=2b00 is reserved. But with MSA/MSB/ELS all set to 0, that > means channel disabled. > > But I think you are right, putting the last paragraph into the clearing of > MSA/MSB/ELS is better, as below: > > 250 /* > 251 * polarity settings will enabled/disable output status > 252 * immediately, so if the channel is disabled, need to > 253 * make sure MSA/MSB/ELS are set to 0 which means channel > 254 * disabled. > 255 */ > 256 val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > 257 val &= ~(PWM_IMX_TPM_CnSC_ELS | PWM_IMX_TPM_CnSC_MSA | > 258 PWM_IMX_TPM_CnSC_MSB); > 259 if (state->enabled) { > > > 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%7Ceb > > > 28bb96ee9844ca92a908d6bccdd40d%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > 5%7C0%7C0%7C636903989547571591&sdata=tnQGymmW1zZZjcSWm > > W40RUZiuQv0lpT2R8mj3znRmWE%3D&reserved=0 |
