On Thu, Feb 14, 2019 at 2:04 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> Hello,
>
> On Thu, Feb 14, 2019 at 01:25:27PM +0530, Yash Shah wrote:
> > On Wed, Feb 13, 2019 at 4:05 PM Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > > On Wed, Feb 13, 2019 at 02:56:18PM +0530, Yash Shah wrote:
> > > > +static int pwm_sifive_enable(struct pwm_chip *chip, struct pwm_device *dev,
> > > > + bool enable)
> > > > +{
> > > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > > + u32 val;
> > > > + int ret;
> > > > +
> > > > + if (enable) {
> > > > + ret = clk_enable(pwm->clk);
> > > > + if (ret) {
> > > > + dev_err(pwm->chip.dev, "Enable clk failed:%d\n", ret);
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + if (enable)
> > > > + val |= BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > > + else
> > > > + val &= ~BIT(PWM_SIFIVE_PWMCFG_EN_ALWAYS);
> > > > +
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + if (!enable)
> > > > + clk_disable(pwm->clk);
> > >
> > > A disabled PWM is supposed to output an inactive signal. If the PWM runs
> > > at (near) 100% and you disable it, does it reliably give that inactive
> > > signal after completing the currently running period?
> >
> > Yes, you are right, it just freezes at that state (100%).
> > What if I set duty cycle = 0 if (!state->enabled) before disabling the PWM?
>
> Then you only need to be sure that the inactive level is already latched
> to the pwmcmpXip output (which should only need one clock cycle if I'm
> not mistaken) before disabling the clock.
>
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int pwm_sifive_apply(struct pwm_chip *chip, struct pwm_device *dev,
> > > > + struct pwm_state *state)
> > > > +{
> > > > + struct pwm_sifive_ddata *pwm = pwm_sifive_chip_to_ddata(chip);
> > > > + unsigned int duty_cycle;
> > > > + u32 frac, val;
> > > > + struct pwm_state cur_state;
> > > > + bool enabled;
> > > > + int ret;
> > > > +
> > > > + pwm_get_state(dev, &cur_state);
> > > > + enabled = cur_state.enabled;
> > > > +
> > > > + if (state->polarity != PWM_POLARITY_INVERSED)
> > > > + return -EINVAL;
> > > > +
> > > > + if (state->period != cur_state.period) {
> > > > + if (pwm->user_count != 1)
> > > > + return -EINVAL;
> > >
> > > I think we need locking here. Consider two pwm users on two CPUs:
> > >
> > > CPU1 CPU2
> > > pwm_sifive_apply(pwm0, period=A, ...)
> > > check user_count==1 -> good
> > > ... pwm1 = pwm_get(...)
> > > ... pwm_sifive_apply(pwm1, period=B...)
> > > ... configure based on B
> > > pwm_sifive_update_clock()
> >
> > mutex_lock();
> > if (pwm->user_count != 1)
> > return -EINVAL;
> > mutex_unlock();
> > Something like this?
>
> No, the lock needs to protect more. You must at least cover increasing
> and decreasing of user_count and you must hold the lock until the period
> update is completed.
Got your point. Will use locks at appropriate places
>
> > > Also I wonder if we should change the period if the user requested
> > > enabled=false.
> >
> > You want me to NOT update period if enabled=false, right?
>
> I don't know for sure. Given that period is shared for all four PWM
> outputs it might be sensible to change it at least in a shadow variable
> and only do it when actually needed. (But maybe we can postpone that as
> it doesn't matter for correctness of the driver.)
>
> The question here is: In the following snippet:
>
> pwm0 = pwm_get(... the first pwm ...)
>
> pwm_apply_state(pwm0, { .enabled = true, .period = 4000 });
> pwm_apply_state(pwm0, { .enabled = false, .period = 8000 });
>
> pwm1 = pwm_get(... the second pwm ...)
> pwm_apply_state(pwm1, { .enabled = true, .period = 4000 });
> pwm_apply_state(pwm0, { .enabled = true, .period = 8000 });
>
> Which of the two last commands should fail?
>
> > > > + pwm->real_period = state->period;
> > > > + pwm_sifive_update_clock(pwm, clk_get_rate(pwm->clk));
> > >
> > > If you change from
> > >
> > > .period = A
> > > .duty_cycle = B
> > >
> > > to
> > >
> > > .period = C
> > > .duty_cycle = D
> > >
> > > the output pin might see a period with
> > >
> > > .period = C
> > > .duty_cycle = B
> > >
> > > right? I think this is not fixable, but this needs a prominent comment.
> >
> > Good point. Is the below comment good enough?
> > /* When changing both duty cycle and period, the old duty cycle might
> > be active with new the period settings for a period */
>
> I'd add some blame on the hardware. Something like:
>
> /*
> * When changing both duty cycle and period, we cannot prevent in
> * software that the output might produce a period with mixed
> * settings (new period length and old duty cycle).
> */
>
> I'd say it makes sense to put this information at the top of the driver
> to have it in a prominent place. Also point out the inability to provide
> 100% duty cycle and that the hardware is limited to inverted output.
> Then all limitations are summarized in a single place.
Sure, will do as you suggested.
>
> Maybe this mismatch could be made less likely by changing the order of
> the register accesses and a delay depending on pwms and old and new
> settings. But I'd say this is too much for now and can be addressed
> later when and if necessary.
>
> > > > + }
> > > > +
> > > > + if (!state->enabled && enabled) {
> > > > + ret = pwm_sifive_enable(chip, dev, false);
> > > > + if (ret)
> > > > + return ret;
> > > > + enabled = false;
> > > > + }
> > > > +
> > > > + duty_cycle = state->duty_cycle;
> > > > + frac = div_u64((u64)duty_cycle * (1 << PWM_SIFIVE_CMPWIDTH) +
> > > > + (1 << PWM_SIFIVE_CMPWIDTH) / 2, state->period);
> > > > + /* The hardware cannot generate a 100% duty cycle */
> > >
> > > @Thierry: Do you consider this bad enough that pwm_apply_state should
> > > fail if 100% is requested?
>
> This question is still open.
>
> > > > + frac = min(frac, (1U << PWM_SIFIVE_CMPWIDTH) - 1);
> > > > +
> > > > + val = readl(pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > + val |= BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > > > + writel(val, pwm->regs + PWM_SIFIVE_PWMCFG);
> > > > +
> > > > + writel(frac, pwm->regs + PWM_SIFIVE_PWMCMP0 + dev->hwpwm *
> > > > + PWM_SIFIVE_SIZE_PWMCMP);
> > > > +
> > > > + val &= ~BIT(PWM_SIFIVE_PWMCFG_DEGLITCH);
> > >
> > > Doesn't that come too early? I thought the right thing was to keep it
> > > set all the time. With this code I think you might see a duty cycle of
> > > 50 when going from 60 to 40.
> >
> > We cannot set it all the time.
> > Setting it all the time makes every alternate period to remain high
> > (latched state).
> > As per the manual, it needs to be set when reprogramming and must be
> > cleared afterwards.
>
> I didn't find this in the manual. When looking at Figure 6 and the
> description of pwmdeglitch I have the impression that your statement is
> wrong.
>
> Setting pwmdeglitch only prevents the output from getting low during a
> period, it can only go low when pwms overflows (i.e. the start of a
> period). That's exactly what we want. Where is the misunderstanding?
>
> If however you clear pwmdeglitch after an update, consider the following
> series of events:
>
> - Assume pwmcmpX is 0x4000 and pwms is 0x5000, so pwmcmpXip is high.
> - You set pwmdeglitch and change pwmcmpX to 0x8000 while pwms advanced
> only little. Then pwmcmpXip remains high.
> - Then you clear pwmdeglitch at say pwms = 0x5020, this makes pwmcmpXip
> fall which we should prevent.
>
> Also note that when setting pwmdeglitch while configuring pwm0---if it
> really had the behaviour you pointed out---would interfere with the
> maybe running pwm1.
>
> So I'm convinced keeping pwmdeglitch active always is the right thing to
> do.
I agree that figure 6 suggests that pwmdeglitch should remain active
through out.
But for some strange reason when I set deglitch bit, I am seeing every
alternate pwm cycle to remain high (latched) unless I clear the
deglitch bit.
I am debugging this issue however is it ok if I remove deglitch logic
from driver until this issue has been root caused?
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
