> -----Original Message----- > From: Krzysztof Kozlowski <krzk@xxxxxxxxxx> > Sent: 01 October 2024 15:30 > To: Inbaraj E <inbaraj.e@xxxxxxxxxxx>; 'Stephen Boyd' > <sboyd@xxxxxxxxxx>; alim.akhtar@xxxxxxxxxxx; cw00.choi@xxxxxxxxxxx; > linux-clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-samsung- > soc@xxxxxxxxxxxxxxx; mturquette@xxxxxxxxxxxx; s.nawrocki@xxxxxxxxxxx > Cc: pankaj.dubey@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx > Subject: Re: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical > > On 01/10/2024 11:24, Inbaraj E wrote: > >>>>>>>> CSI stop streaming through pm_runtime_put system is getting > >> halted. > >>>>>>>> So marking PLL_CAM_CSI as critical to prevent disabling. > >>>>>>>> > >>>>>>>> Signed-off-by: Inbaraj E <inbaraj.e@xxxxxxxxxxx> > >>>>>>>> --- > >>>>>>> > >>>>>>> Please add a fixes tag. Although this is likely a band-aid fix > >>>>>>> because marking something critical leaves it enabled forever. > >>>>>> > >>>>>> Sure, will add fixes tag. As per HW manual, this PLL_CAM_CSI is > >>>>>> supplying clock even for CMU SFR access of CSI block, so we can't > >>>>>> gate this. > >>>>> > >>>>> Hm, I am not so sure. The CMU driver should just take appropriate > clock. > >>>>> Sprinkling CLK_CRITICAL looks as substitute of missing clock > >>>>> handling/ > >>>> > >>>> As per HW design, PLL_CAM_CSI is responsible for suppling clock to > >>>> CSI SFR, CMU SFR and some internal block of CAM_CSI. In this some > >>>> of the clock is not handled by any driver but it is required for > >>>> CSI to work properly. For example CSI NOC clock. So this is the > >>>> reason we are > >> marking PLL_CAM_CSI as critical. > >>>> > >>> > >>> This is clock hierarchy for CMU_CAM_CSI block. > >>> > >>> PLL_CAM_CSI -----> DIVIDER --------> CSI_SFR clock > >>> | > >>> |----> DIVIDER --------> CMU_SFR clock > >>> | > >>> |----> DIVIDER --------> CSI NOC clock. > >>> > >> > >> And what is the problem in adding proper handling in the driver? You > >> just described case valid for 99% of SoC components. > > > > Hi Kryzstof, > > > > Sorry, but it seems I was not able to explain the issue. Let me add > > more > > details: > > So for CSI IP we have two clocks as ACLK and PCLK which needs to be > > handled by the driver during start and stop streaming. > > > > In BLK_CSI we have CSI IP along with other bunch supporting modules > > such as CMU_CSI, NOC_CSI, CSI_SFR. For all these components of BLK_CSI > > we have a single top level parent PLL clock as PLL_CAM_CSI. > > > > Now if we look into CSI driver perspective it needs only ACLK and PCLK > > clocks for it's operations. But to access CMU SFRs (including > > ACLK/PCLK or any other CMU SFR of BLK_CSI) we need parent clock keep > > supplying clocks. While we try to gate ACLK clock, due to propagation > > logic of clock gating the CCF scans all the clocks from leaf level to > > the parent clock and tries to gate clocks if enable/disable ops is > > valid for any such clock. > > > > Issue here is that we are trying to gate PLL_CAM_CSI which itself is > > accessible only when this clock is enabled. In fact none of CMU_SFR > > will be accessible as soon as PLL_CAM_CSI is gated. CSI driver is not > > intended > > Obviously, but your CMU is taking the necessary clock and enabled it so what > is the problem? > > > to gate this PLL clock but only the leaf level clock which is > > supplying to CSI IP. So in absence of any alternate source of clock > > hierarchy which can supply clock for CMU_CSI we can't gate PLL_CAM_CSI. > > > > Please let us know if you have any other queries why we are insisting > > on marking PLL_CAM_CSI as CRITICAL clock. > > This is so far quite obvious - just like in all other cases, you need the top clock > taken by proper driver. I don't think you are looking at right drivers and right > problem here. > Hi Krzysztof, In this case, platform driver need to get this PLL clock and keep it enabled always. As PLL_CAM_CSI is source clock for accessing CMU registers of CSI block, and PLL_CAM_CSI itself lies in CSI_CMU, driver need to prepare and keep it enabled always. This way other PCLK and ACLK clocks can be gated. But the PLL_CAM_CSI which is parent of the PCLK and ACLK gate clock won't be disabled. Hope this should not be a concern. Regards, Inbaraj E > Best regards, > Krzysztof
