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)? > > [1] > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20220504184406.93788-1-biju.das.jz@xxxxxxxxxxxxxx/ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
