Hi Geert and Philipp, > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to > deassert/assert reset line > > Hi Biju, > > On Mon, Jul 18, 2022 at 12:12 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx> > wrote: > > > Subject: Re: [PATCH v11 2/5] media: renesas: vsp1: Add support to > > > deassert/assert reset line > > > > > > 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? > > > > OK. > > > > > > > > > > > > > > > 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. > > > > For RZ/G1N SoC's like Geert mentioned in [1], calling > > reset_control_status() only once fixes the issue. So strictly speaking > polling is not required. > > > > @Geert Uytterhoeven, Please share your opinion on this. > > According to that thread, it also works without reading the register, > when adding a small delay() (to the vsp driver)? What about using the below logic? If ((reset_control_status() == 0) && udelay(x)) dev_warn(dev, "Deassert failed"); This makes system wake up after STR faster compared to udelay. Currently with first status readback() is sufficient for fixing the issue. But this logic is tested only on few boards. Adding a warn message to this logic, to check if any of the boards is returning reset_control_status() as "0". Are we ok with this? Please share your views. Cheers, Biju
