Re: [PATCH v1] clk: qcom: lpass: Add CLK_IGNORE_UNUSED for lpass clocks

[Taniya Das <tdas@codeaurora.org>]

Hello Stephen,

On 1/19/2019 12:31 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2019-01-17 03:19:22)
>>
>>
>> On 1/15/2019 3:55 AM, Stephen Boyd wrote:
>>> Quoting Taniya Das (2019-01-13 22:12:39)
>>>>
>>>>
>>>> On 1/8/2019 2:34 AM, Stephen Boyd wrote:
>>>>>
>>>>> As far as I know, I'm not suggesting the use of CLK_IS_CRITICAL here.
>>>>> But removing CLK_IS_CRITICAL and relying on some random bootloader
>>>>> behavior also looks wrong. Can you clarify what's going on?
>>>>>
>>>>
>>>> To enable LPASS clocks the requirement is to enable the GCC_LPASS_SWAY
>>>> clock.
>>>> 1) If the LPASS drivers are enabled/probed before the clock late init
>>>> the client would take care to maintain the dependency to enable the
>>>> GCC_LPASS_SWAY clock before enabling the LPASS clocks.
>>>>
>>>> 2) There could be a condition where the LPASS drivers would probe/init
>>>> later the clock late_init. When the clock_late_init would try to access
>>>> the LPASS clocks, since we cannot maintain the dependency this access
>>>> would fail. To avoid this the earlier patch has made the GCC_LPASS_SWAY
>>>> clock as CRITICAL.
>>>>
>>>> 3) Marking the GCC_LPASS_SWAY clock as CRITICAL has a issue, in the case
>>>> where the LPASS subsystem would be restarted due to some critical
>>>> failure on LPASS. Toggling the restart register of LPASS would clear the
>>>> hardware state of this clock and thus the next access of the LPASS
>>>> clocks would result in failure of the system.
>>>>
>>>> 4) To avoid issues happening in (2) and (3) all the LPASS clocks chould
>>>> be safely marked as CLK_IGNORE_UNUSED. And lpass drivers would take care
>>>> of the dependency to enable the required clocks.
>>>>
>>>
>>> Ok, so why can't we enable/disable the lpass sway clk in the
>>> prepare/unprepare phase of the lpass clk driver paths? Or why can't we
>>> forcibly enable this lpass sway clk after the reset is deasserted? Which
>>> clk controller is the reset part of? GCC or LPASS?
>>
>> It is part of Always On Subsystem.
> 
> Ok. Is this merged upstream?
> 
>>
>>>
>>> It still sounds like the LPASS clk driver isn't handling dependencies it
>>> has on accessing registers, but maybe we can get away with not handling
>>> the dependency still if we make the reset "do the right thing" and turn
>>> the clk back on so it stays "critical".
>>>
>>
>> This is a reset from hardware and it does not bring back the clock to
>> the previous state and so we can not mark it "critical". I would submit
>> the next series with comments updated. Please let me know in case you
>> have any comments.
>>
>>
> 
> Can we have the always on subsystem reset code go hit this clk enable
> bit back on?

There is no code, it is a reset from hardware.

  And also have the LPASS clk driver get this GCC sway clk
> and enable it during probe? Maybe we need to get some sort of API in the
> clk provider layer that can tell us that the clk state has changed now
> and it needs to be restored. I haven't thought about it deeply but that
> may be the best solution here.
> 

Would it be possible to go about with the current patch and then have it 
updated with the possible solutions.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.

--