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

[Krzysztof Kozlowski <krzk@kernel.org>]

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