Messages in this thread | | | Date | Thu, 17 Nov 2022 22:03:13 +0000 | From | Conor Dooley <> | Subject | Re: [PATCH v12 1/2] pwm: add microchip soft ip corePWM driver |
| |
On Thu, Nov 17, 2022 at 10:04:33PM +0100, Uwe Kleine-König wrote: > On Thu, Nov 17, 2022 at 05:38:26PM +0000, Conor Dooley wrote: > > On Thu, Nov 17, 2022 at 05:49:50PM +0100, Uwe Kleine-König wrote: > > > Hello Conor, > > > > Hello Uwe, > > > > > On Thu, Nov 10, 2022 at 09:35:12AM +0000, Conor Dooley wrote: > > > > [...] > > > > + > > > > +static void mchp_core_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + bool enable, u64 period) > > > > +{ > > > > + struct mchp_core_pwm_chip *mchp_core_pwm = to_mchp_core_pwm(chip); > > > > + u8 channel_enable, reg_offset, shift; > > > > + > > > > + /* > > > > + * There are two adjacent 8 bit control regs, the lower reg controls > > > > + * 0-7 and the upper reg 8-15. Check if the pwm is in the upper reg > > > > + * and if so, offset by the bus width. > > > > + */ > > > > + reg_offset = MCHPCOREPWM_EN(pwm->hwpwm >> 3); > > > > + shift = pwm->hwpwm & 7; > > > > + > > > > + channel_enable = readb_relaxed(mchp_core_pwm->base + reg_offset); > > > > + channel_enable &= ~(1 << shift); > > > > + channel_enable |= (enable << shift); > > > > + > > > > + writel_relaxed(channel_enable, mchp_core_pwm->base + reg_offset); > > > > + mchp_core_pwm->channel_enabled &= ~BIT(pwm->hwpwm); > > > > + mchp_core_pwm->channel_enabled |= enable << pwm->hwpwm; > > > > + > > > > + /* > > > > + * Notify the block to update the waveform from the shadow registers. > > > > + * The updated values will not appear on the bus until they have been > > > > + * applied to the waveform at the beginning of the next period. We must > > > > + * write these registers and wait for them to be applied before > > > > + * considering the channel enabled. > > > > + * If the delay is under 1 us, sleep for at least 1 us anyway. > > > > + */ > > > > + if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) { > > > > + u64 delay; > > > > + > > > > + delay = div_u64(period, 1000u) ? : 1u; > > > > + writel_relaxed(1U, mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD); > > > > + usleep_range(delay, delay * 2); > > > > + } > > > > > > In some cases the delay could be prevented. e.g. when going from one > > > disabled state to another. If you don't want to complicate the driver > > > here, maybe point it out in a comment at least? > > > > Maybe this is my naivity talking, but I'd rather wait. Is there not the > > chance that we re-enter pwm_apply() before the update has actually gone > > through? > > My idea was to do something like that: > > int mchp_core_pwm_apply(....) > { > if (mchp_core_pwm->sync_update_mask & (1 << pwm->hwpwm)) { > /* > * We're still waiting for an update, don't > * interfer until it's completed. > */ > while (readl_relaxed(mchp_core_pwm->base + MCHPCOREPWM_SYNC_UPD)) { > cpu_relax(); > if (waited_unreasonably_long()) > return -ETIMEOUT; > } > } > > update_period_and_duty(...); > return 0; > } > > This way you don't have to wait at all if the calls to pwm_apply() are > infrequent. Of course this only works this way, if you can determine if > there is a pending update.
Ah I think I get what you mean now about waiting for completion & reading the bit. I don't know off the top of my head if that bit is readable. Docs say that they're R/W but I don't know if that means that an AXI read works or if the value is actually readable. I'll try something like this if I can.
> From a simplicity POV always waiting is fine. But if you consider > periods of say 5s, letting a call to pwm_apply() do a sleep between 5 > and 10 seconds at the end is quite long and blocks the caller > considerably.
Yeah, I know. At the end of the day, you're the one familiar with what PWM consumers expect. If things go the wait-but-maybe-exit-early direction I think I'll add something to the limitations to cover that.
> > IIRC, but I'll have to confirm it, when the "shadow registers" are > > enabled reads show the values that the hardware is using rather than the > > values that are queued in the shadow registers. I'd rather avoid that > > sort of mess and always sleep. > > > > Now that I think of it, the reason I moved to unconditionally sleeping > > was that if I turned on the PWM debugging it'd get tripped up. When it > > tried to read the state, it got the old one rather than what'd just been > > written. > > > > Pasting my comment from above: > > > > + /* > > > > + * Notify the block to update the waveform from the shadow registers. > > > > + * The updated values will not appear on the bus until they have been > > > > By "bus" in this statement, I meant on the AXI/AHB etc bus that the IP > > core is connected to the CPUs on rather than the output. Perhaps my > > wording of the comment could be improved and replace the word "bus" with > > some wording containing "CPU" instead. "The updated values will not > > appear to the CPU until" maybe. > > I'd write: Reading the registers yields the currently implemented > settings, the new ones are only readable once the current period ended.
Cool, will use that so.
> > I can also add some words relating to unconditionally sleeping w.r.t to > > disabled states. > > > > > > + * applied to the waveform at the beginning of the next period. We must > > > > + * write these registers and wait for them to be applied before > > > > + * considering the channel enabled. > > > > + * If the delay is under 1 us, sleep for at least 1 us anyway. > > > > + */ > > > > > It's not well defined if pwm_apply should only return when the new > > > setting is actually active. (e.g. mxs doesn't wait) > > > So I wonder: Are there any hardware restrictions between setting the > > > SYNC_UPD flag and modifying the registers for duty and period? (I assume > > > writing a new duty and period might then result in a glitch if the > > > period just ends between the two writes.) Can you check if the hardware > > > waits on such a completion, e.g. by reading that register? > > > > Not entirely sure by what you mean: "waits on such a completion". > > I wanted to say that it's okish to return from .apply() without the > sleep and so return to the caller before the requested setting appears > on the output. At least your driver wouldn't be the first to do it that > way.
I'd be more comfortable with it if the readable registers didn't hold the old value.
> > The hardware updates the registers at the first end-of-period after > > SYNC_UPD is set. Don't write the bit, nothing happens. From the docs: > > > > > > A shadow register holds all values and writes them when the SYNC_UPDATE > > > > register is set to 1. In other words, for all channel synchronous > > > > updates, write a "1" to the SYNC_UPDATE register after writing to all > > > > the channel registers. > > > > The docs also say: > > > > SYNC_UPDATE: When this bit is set to "1" and SHADOW_REG_EN > > > > is selected, all POSEDGE and NEGEDGE registers are updated > > > > synchronously. Synchronous updates to the PWM waveform occur only > > > > when SHADOW_REG_EN is asserted and SYNC_UPDATE is set to “1”. > > > > > > > > When this bit is set to "0", all the POSEDGE and NEGEDGE registers > > > > are updated asynchronously > > > > The second statement is at best vague (if the this bit in "when this > > bit" refers to the bit in SHADOW_REG_EN) or contradictory at worse. > > I suspect it's the former meaning, as shadow registers are a per-channel > > thing. I suppose I have to go get some docs changed, **sigh**. It > > doesn't make all that much sense to me, SHADOW_REG_EN is a RTL parameter > > not a register that can be accessed from the AXI interface. > > > > Anyways, back to the topic at hand.. if you were to do the following > > (in really pseudocode form..): > > write(SYNC_UPD) > > write(period) > > <end-of-period> > > write(duty) > > > > Then the duty cycle would not get updated, ever. At least, per doc > > comment #1 & my "experimental" data. The RTL is rather dumb, since > > AFAICT, this is meant to be cheap to implement in FPGA fabric. > > Hence the default core configuration option is no shadow registers > > & just immediately updates the output, waveform glitches be damned. > > > > Hopefully that all helps? > > I already understood it that way. I hope I was able to clarify my > thoughts above.
Yeah, I think you did!
Thanks again, Conor.
| |