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. Best regards, Krzysztof
