clk: samsung: fsd: Mark PLL_CAM_CSI as critical

Commit Message

Inbaraj E Sept. 17, 2024, 10:10 a.m. UTC

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 <[e-mail]>
---
 drivers/clk/samsung/clk-fsd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Stephen Boyd Sept. 19, 2024, 10:21 a.m. UTC | #1

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 <[e-mail]>
> ---

Please add a fixes tag. Although this is likely a band-aid fix because
marking something critical leaves it enabled forever.

>  drivers/clk/samsung/clk-fsd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 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,

Please add a comment indicating that this clk can never turn off because
<insert reason here>.

Inbaraj E Sept. 19, 2024, 11:33 a.m. UTC | #2

> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: 19 September 2024 15:51
> To: Inbaraj E <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]; Inbaraj E
> <[e-mail]>
> 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 <[e-mail]>
> > ---
> 
> 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.

> 
> >  drivers/clk/samsung/clk-fsd.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > 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,
> 
> Please add a comment indicating that this clk can never turn off because
> <insert reason here>.

Sure, will post v2 after adding comment explaining reason behind
marking this clock as critical.

Regards,
Inbaraj E

Krzysztof Kozlowski Sept. 19, 2024, 12:03 p.m. UTC | #3

On 19/09/2024 13:33, Inbaraj E wrote:
> 
> 
>> -----Original Message-----
>> From: Stephen Boyd <[e-mail]>
>> Sent: 19 September 2024 15:51
>> To: Inbaraj E <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; linux-
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Cc: [e-mail]; [e-mail]; Inbaraj E
>> <[e-mail]>
>> 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 <[e-mail]>
>>> ---
>>
>> 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/


Best regards,
Krzysztof

Inbaraj E Sept. 20, 2024, 4:04 a.m. UTC | #4

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 19 September 2024 17:33
> To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-samsung-
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Sent: 19 September 2024 15:51
> >> To: Inbaraj E <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> linux- [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Cc: [e-mail]; [e-mail]; Inbaraj E
> >> <[e-mail]>
> >> 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 <[e-mail]>
> >>> ---
> >>
> >> 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.

> 
> 
> Best regards,
> Krzysztof

Inbaraj E Sept. 20, 2024, 4:15 a.m. UTC | #5

> -----Original Message-----
> From: Inbaraj E <[e-mail]>
> Sent: 20 September 2024 09:35
> To: 'Krzysztof Kozlowski' <[e-mail]>; 'Stephen Boyd'
> <[e-mail]>; '[e-mail]'
> <[e-mail]>; '[e-mail]'
> <[e-mail]>; '[e-mail]' <linux-
> [e-mail]>; '[e-mail]' <linux-
> [e-mail]>; '[e-mail]' <linux-
> [e-mail]>; '[e-mail]'
> <[e-mail]>; '[e-mail]'
> <[e-mail]>
> Cc: '[e-mail]' <[e-mail]>;
> '[e-mail]' <[e-mail]>
> Subject: RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical
> 
> 
> 
> > -----Original Message-----
> > From: Krzysztof Kozlowski <[e-mail]>
> > Sent: 19 September 2024 17:33
> > To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
> > <[e-mail]>; [e-mail];
> [e-mail];
> > [e-mail]; [e-mail];
> > linux-samsung- [e-mail]; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; [e-mail]
> > 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 <[e-mail]>
> > >> Sent: 19 September 2024 15:51
> > >> To: Inbaraj E <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > >> linux- [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]
> > >> Cc: [e-mail]; [e-mail]; Inbaraj E
> > >> <[e-mail]>
> > >> 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 <[e-mail]>
> > >>> ---
> > >>
> > >> 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. 

Regards,
Inbaraj E
> >
> >
> > Best regards,
> > Krzysztof

Krzysztof Kozlowski Sept. 20, 2024, 12:36 p.m. UTC | #6

On 20/09/2024 06:15, Inbaraj E wrote:
> 
> 
>> -----Original Message-----
>> From: Inbaraj E <[e-mail]>
>> Sent: 20 September 2024 09:35
>> To: 'Krzysztof Kozlowski' <[e-mail]>; 'Stephen Boyd'
>> <[e-mail]>; '[e-mail]'
>> <[e-mail]>; '[e-mail]'
>> <[e-mail]>; '[e-mail]' <linux-
>> [e-mail]>; '[e-mail]' <linux-
>> [e-mail]>; '[e-mail]' <linux-
>> [e-mail]>; '[e-mail]'
>> <[e-mail]>; '[e-mail]'
>> <[e-mail]>
>> Cc: '[e-mail]' <[e-mail]>;
>> '[e-mail]' <[e-mail]>
>> Subject: RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical
>>
>>
>>
>>> -----Original Message-----
>>> From: Krzysztof Kozlowski <[e-mail]>
>>> Sent: 19 September 2024 17:33
>>> To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
>>> <[e-mail]>; [e-mail];
>> [e-mail];
>>> [e-mail]; [e-mail];
>>> linux-samsung- [e-mail]; [e-mail];
>>> [e-mail]
>>> Cc: [e-mail]; [e-mail]
>>> 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 <[e-mail]>
>>>>> Sent: 19 September 2024 15:51
>>>>> To: Inbaraj E <[e-mail]>; [e-mail];
>>>>> [e-mail]; [e-mail]; [e-mail];
>>>>> linux- [e-mail]; [e-mail];
>>>>> [e-mail]; [e-mail]
>>>>> Cc: [e-mail]; [e-mail]; Inbaraj E
>>>>> <[e-mail]>
>>>>> 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 <[e-mail]>
>>>>>> ---
>>>>>
>>>>> 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.

Best regards,
Krzysztof

Inbaraj E Oct. 1, 2024, 9:24 a.m. UTC | #7

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 20 September 2024 18:07
> To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-samsung-
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> >> Sent: 20 September 2024 09:35
> >> To: 'Krzysztof Kozlowski' <[e-mail]>; 'Stephen Boyd'
> >> <[e-mail]>; '[e-mail]'
> >> <[e-mail]>; '[e-mail]'
> >> <[e-mail]>; '[e-mail]' <linux-
> >> [e-mail]>; '[e-mail]' <linux-
> >> [e-mail]>; '[e-mail]' <linux-
> >> [e-mail]>; '[e-mail]'
> >> <[e-mail]>; '[e-mail]'
> >> <[e-mail]>
> >> Cc: '[e-mail]' <[e-mail]>;
> >> '[e-mail]' <[e-mail]>
> >> Subject: RE: [PATCH] clk: samsung: fsd: Mark PLL_CAM_CSI as critical
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: Krzysztof Kozlowski <[e-mail]>
> >>> Sent: 19 September 2024 17:33
> >>> To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
> >>> <[e-mail]>; [e-mail];
> >> [e-mail];
> >>> [e-mail]; [e-mail];
> >>> linux-samsung- [e-mail]; [e-mail];
> >>> [e-mail]
> >>> Cc: [e-mail]; [e-mail]
> >>> 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 <[e-mail]>
> >>>>> Sent: 19 September 2024 15:51
> >>>>> To: Inbaraj E <[e-mail]>; [e-mail];
> >>>>> [e-mail]; [e-mail]; linux-
> [e-mail];
> >>>>> linux- [e-mail]; [e-mail];
> >>>>> [e-mail]; [e-mail]
> >>>>> Cc: [e-mail]; [e-mail]; Inbaraj E
> >>>>> <[e-mail]>
> >>>>> 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 <[e-mail]>
> >>>>>> ---
> >>>>>
> >>>>> 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

Krzysztof Kozlowski Oct. 1, 2024, 10 a.m. UTC | #8

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 <[e-mail]>
>>>>>>>> ---
>>>>>>>
>>>>>>> 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

Inbaraj E Oct. 10, 2024, 10:45 a.m. UTC | #9

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: 01 October 2024 15:30
> To: Inbaraj E <[e-mail]>; 'Stephen Boyd'
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; linux-samsung-
> [e-mail]; [e-mail]; [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>> 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

Krzysztof Kozlowski Oct. 21, 2024, 9:50 a.m. UTC | #10

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

Patch

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" };