On Mon, Aug 05, 2019 at 10:12:07AM +0000, Pascal Van Leeuwen wrote: > > From: Antoine Tenart <antoine.tenart@xxxxxxxxxxx> > > On Mon, Aug 05, 2019 at 09:48:13AM +0000, Pascal Van Leeuwen wrote: > > > > On Wed, Jul 31, 2019 at 05:29:19PM +0200, Pascal van Leeuwen wrote: > > > > > > > > > > - /* Release engine from reset */ > > > > > - val = readl(EIP197_PE(priv) + ctrl); > > > > > - val &= ~EIP197_PE_ICE_x_CTRL_SW_RESET; > > > > > - writel(val, EIP197_PE(priv) + ctrl); > > > > > + for (pe = 0; pe < priv->config.pes; pe++) { > > > > > + base = EIP197_PE_ICE_SCRATCH_RAM(pe); > > > > > + pollcnt = EIP197_FW_START_POLLCNT; > > > > > + while (pollcnt && > > > > > + (readl_relaxed(EIP197_PE(priv) + base + > > > > > + pollofs) != 1)) { > > > > > + pollcnt--; > > > > > > > > You might want to use readl_relaxed_poll_timeout() here, instead of a > > > > busy polling. > > > > > > > Didn't know such a thing existed, but I also wonder how appropriate it > > > is in this case, condering it measures in whole microseconds, while the > > > response time I'm expecting here is in the order of a few dozen nano- > > > seconds internally ... i.e. 1 microsecond is already a *huge* overkill. > > > > > > The current implementation runs that loop for only 16 iterations which > > > should be both more than sufficient (it probably could be reduced > > > further, I picked 16 rather arbitrarily) and at the same time take so > > > few cycles on the CPU that I doubt it is worthwhile to reschedule/ > > > preempt/whatever? > > > > Your choice, I was just making a suggestion :) > > > After reading the implementation code, I think using it with a > sleep_us value of 0 and a time-out value of 1 may be reasonable here, > as time-out means some serious error that shouldn't normally happen. > I just wouldn't want any microsecond delays for the "normal" case. Using this function that is designed to sleep with a delay of 0 and designed to timeout with a value of 1 does not seem to follow what the function is designed for :) It could work but I would suggest to keep the polling as it is in the patch in this case. Thanks, Antoine -- Antoine Ténart, Bootlin Embedded Linux and Kernel engineering https://bootlin.com