| Message ID | 20240917101016.23238-1-inbaraj.e@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: samsung: fsd: Mark PLL_CAM_CSI as critical | expand |
> -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: 20 September 2024 18:07 > 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 20/09/2024 06:15, Inbaraj E wrote: > > > > > >> -----Original Message----- > >> From: Inbaraj E <inbaraj.e@samsung.com> > >> Sent: 20 September 2024 09:35 > >> To: 'Krzysztof Kozlowski' <krzk@kernel.org>; 'Stephen Boyd' > >> <sboyd@kernel.org>; 'alim.akhtar@samsung.com' > >> <alim.akhtar@samsung.com>; 'cw00.choi@samsung.com' > >> <cw00.choi@samsung.com>; 'linux-clk@vger.kernel.org' <linux- > >> clk@vger.kernel.org>; 'linux-kernel@vger.kernel.org' <linux- > >> kernel@vger.kernel.org>; 'linux-samsung-soc@vger.kernel.org' <linux- > >> samsung-soc@vger.kernel.org>; 'mturquette@baylibre.com' > >> <mturquette@baylibre.com>; 's.nawrocki@samsung.com' > >> <s.nawrocki@samsung.com> > >> Cc: 'pankaj.dubey@samsung.com' <pankaj.dubey@samsung.com>; > >> 'gost.dev@samsung.com' <gost.dev@samsung.com> > >> Subject: RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical > >> > >> > >> > >>> -----Original Message----- > >>> From: Krzysztof Kozlowski <krzk@kernel.org> > >>> Sent: 19 September 2024 17:33 > >>> 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 19/09/2024 13:33, Inbaraj E wrote: > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: Stephen Boyd <sboyd@kernel.org> > >>>>> Sent: 19 September 2024 15:51 > >>>>> To: Inbaraj E <inbaraj.e@samsung.com>; alim.akhtar@samsung.com; > >>>>> cw00.choi@samsung.com; krzk@kernel.org; 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; Inbaraj E > >>>>> <inbaraj.e@samsung.com> > >>>>> 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@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 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
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. Best regards, Krzysztof
> -----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
On 10/10/2024 12:45, Inbaraj E wrote: >>> 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. > Yes, that's expected. It properly models the hierarchy and consumers of clocks, while keeping something as critical does not model the consumers. Best regards, Krzysztof
diff --git a/drivers/clk/samsung/clk-fsd.c b/drivers/clk/samsung/clk-fsd.c index 6f984cfcd33c..b1764aab9429 100644 --- a/drivers/clk/samsung/clk-fsd.c +++ b/drivers/clk/samsung/clk-fsd.c @@ -1637,8 +1637,9 @@ static const struct samsung_pll_rate_table pll_cam_csi_rate_table[] __initconst }; static const struct samsung_pll_clock cam_csi_pll_clks[] __initconst = { - PLL(pll_142xx, 0, "fout_pll_cam_csi", "fin_pll", - PLL_LOCKTIME_PLL_CAM_CSI, PLL_CON0_PLL_CAM_CSI, pll_cam_csi_rate_table), + __PLL(pll_142xx, 0, "fout_pll_cam_csi", "fin_pll", + CLK_GET_RATE_NOCACHE | CLK_IS_CRITICAL, PLL_LOCKTIME_PLL_CAM_CSI, + PLL_CON0_PLL_CAM_CSI, pll_cam_csi_rate_table), }; PNAME(mout_cam_csi_pll_p) = { "fin_pll", "fout_pll_cam_csi" };
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@samsung.com> --- drivers/clk/samsung/clk-fsd.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)