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

[Inbaraj E]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> Sent: 20 September 2024 18:07
> 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 20/09/2024 06:15, Inbaraj E wrote:
> >
> >
> >> -----Original Message-----
> >> From: Inbaraj E <inbaraj.e@xxxxxxxxxxx>
> >> Sent: 20 September 2024 09:35
> >> To: 'Krzysztof Kozlowski' <krzk@xxxxxxxxxx>; 'Stephen Boyd'
> >> <sboyd@xxxxxxxxxx>; 'alim.akhtar@xxxxxxxxxxx'
> >> <alim.akhtar@xxxxxxxxxxx>; 'cw00.choi@xxxxxxxxxxx'
> >> <cw00.choi@xxxxxxxxxxx>; 'linux-clk@xxxxxxxxxxxxxxx' <linux-
> >> clk@xxxxxxxxxxxxxxx>; 'linux-kernel@xxxxxxxxxxxxxxx' <linux-
> >> kernel@xxxxxxxxxxxxxxx>; 'linux-samsung-soc@xxxxxxxxxxxxxxx' <linux-
> >> samsung-soc@xxxxxxxxxxxxxxx>; 'mturquette@xxxxxxxxxxxx'
> >> <mturquette@xxxxxxxxxxxx>; 's.nawrocki@xxxxxxxxxxx'
> >> <s.nawrocki@xxxxxxxxxxx>
> >> Cc: 'pankaj.dubey@xxxxxxxxxxx' <pankaj.dubey@xxxxxxxxxxx>;
> >> 'gost.dev@xxxxxxxxxxx' <gost.dev@xxxxxxxxxxx>
> >> Subject: RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
> >>> Sent: 19 September 2024 17:33
> >>> 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 19/09/2024 13:33, Inbaraj E wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Stephen Boyd <sboyd@xxxxxxxxxx>
> >>>>> Sent: 19 September 2024 15:51
> >>>>> To: Inbaraj E <inbaraj.e@xxxxxxxxxxx>; alim.akhtar@xxxxxxxxxxx;
> >>>>> cw00.choi@xxxxxxxxxxx; krzk@xxxxxxxxxx; linux-
> clk@xxxxxxxxxxxxxxx;
> >>>>> linux- kernel@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx;
> >>>>> mturquette@xxxxxxxxxxxx; s.nawrocki@xxxxxxxxxxx
> >>>>> Cc: pankaj.dubey@xxxxxxxxxxx; gost.dev@xxxxxxxxxxx; Inbaraj E
> >>>>> <inbaraj.e@xxxxxxxxxxx>
> >>>>> Subject: Re: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as
> >>>>> critical
> >>>>>
> >>>>> Quoting Inbaraj E (2024-09-17 03:10:16)
> >>>>>> PLL_CAM_CSI is the parent clock for the ACLK and PCLK in the
> >>>>>> CMU_CAM_CSI block. When we gate ACLK or PCLK, the clock
> >> framework
> >>>>> will
> >>>>>> subsequently disables the parent clocks(PLL_CAM_CSI). Disabling
> >>>>>> PLL_CAM_CSI is causing sytem level halt.
> >>>>>>
> >>>>>> It was observed on FSD SoC, when we gate the ACLK and PCLK
> during
> >>>>>> 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
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.

Regards,
Inbaraj E

> 
> Best regards,
> Krzysztof