Hi, On Mo, 2022-07-18 at 09:46 +0000, Biju Das wrote: > Hi Philipp and Geert, > > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to > > deassert/assert reset line > > > > Hi Philipp, > > > > On Wed, Jul 13, 2022 at 12:32 PM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > > wrote: > > > On Wed, Jul 13, 2022 at 11:27:56AM +0200, Geert Uytterhoeven wrote: > > > [...] > > > > Actually I suggested handling this in the VSP driver, as VSP seems > > > > to be "special". > > > > > > > > > > > > > > [1] > > > > > > So reset_control_status never actually returns 1 and the polling loop > > > is not necessary at all? > > > > > > If it's just the status register read that fixes things for VSP, could > > > it be that the deasserting register write to the reset controller and > > > the following register writes to VSP are not ordered somewhere at the > > > interconnect and the read issued to the reset controller just > > > guarantees that order? > > > > The udelay() also works. > > > > While the reset may be deasserted immediately (at the reset controller > > level), the VSP may need some additional time to settle/initialize (at > > the VSP level). ^ this feels to me like we are blindly applying a workaround for an unknown problem. Is there any way to find out what actually causes this delay (or status readback) to be necessary? Is there something documented, like a certain number of VSP clocks required to internally propagate the reset? > > > > Reset is known to work on other blocks on the same SoC, so that's why I > > suggested handling this in the VSP driver instead, like we already do for > > i2c. > > From the discussion, we agree that the current implementation is good. > > Please correct me if my understanding is wrong. >From my understanding, not quite. At least the timeout poll is unnecessary and misleading. It suggests that reset_control_status() could return 0 at this point, which would be a bug in the reset driver. If reset_control_status() only ever returns 1 and the polling loop ends in the first iteration, you can drop the loop and just read status once. Or use udelay(), at this point I have not enough information to understand which would be more appropriate. regards Philipp
