RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical

["Inbaraj E" <inbaraj.e@samsung.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 01 October 2024 15:30
> To: Inbaraj E <inbaraj.e@samsung.com>; 'Stephen Boyd'
> <sboyd@kernel.org>; alim.akhtar@samsung.com; cw00.choi@samsung.com;
> linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; linux-samsung-
> soc@vger.kernel.org; mturquette@baylibre.com; s.nawrocki@samsung.com
> Cc: pankaj.dubey@samsung.com; gost.dev@samsung.com
> 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@samsung.com>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>> 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