Hi Philipp and Geert, > 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. [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@xxxxxxxxxxxxxx/ Cheers, Biju
