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

From: Stephen Boyd <sboyd@kernel.org>
To: Michael Turquette <mturquette@baylibre.com>,
	Taniya Das <tdas@codeaurora.org>
Cc: Andy Gross <andy.gross@linaro.org>,
	David Brown <david.brown@linaro.org>,
	Rajendra Nayak <rnayak@codeaurora.org>,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
	Taniya Das <tdas@codeaurora.org>
Subject: Re: [PATCH v1] clk: qcom: lpass: Add CLK_IGNORE_UNUSED for lpass clocks
Date: Thu, 20 Dec 2018 13:04:55 -0800	[thread overview]
Message-ID: <154533989563.79149.10213938035592547726@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <1545306385-31240-1-git-send-email-tdas@codeaurora.org>

Quoting Taniya Das (2018-12-20 03:46:25)
> The LPASS clocks has a dependency on the GCC lpass clocks to be enabled
> before accessing them and that was the reason to mark the gcc lpass clocks
> as critical. But in the case where the lpass subsystem would require a
> restart, toggling the lpass reset would from HW clear the SW enable bits
> of the GCC lpass clocks. Thus the next time bringing up the lpass subsystem
> out of reset would fail.
> 
> Allow the lpass clock driver to enable/disable the gcc lpass clocks and
> mark the lpass clocks not be accessed during late_init if no client vote.

You need to add more details here. It feels like you wrote the beginning
of a paragraph and then stopped abruptly, leaving the reader hanging for
the whole story. Why is late_init important? Why do we need to leave
them on from the bootloader? What if the bootloader doesn't leave them
enabled? This is all rather hacky so I'm deeply confused. Does the lpass
driver need to get these gcc clks and enable/prepare them during probe?
But then it needs to also allow a reset happen and change the clk state?

I suspect this situation is circling a larger problem where a reset is
toggled and that changes some clk state without the clk framework
knowing. There's not much we can do about that besides having some
mechanism for the clks to know that their state is now out of sync. If
that can be done on the provider driver side then we should have an
easier time not needing to write a bunch of framework code to handle
this. OMAP folks are dealing with the same problems from what I recall.

> 
> Signed-off-by: Taniya Das <tdas@codeaurora.org>

> @@ -77,6 +81,7 @@
>                 .enable_mask = BIT(0),
>                 .hw.init = &(struct clk_init_data){
>                         .name = "lpass_qdsp6ss_sleep_clk",
> +                       .flags = CLK_IGNORE_UNUSED,

All uses of CLK_IGNORE_UNUSED and CLK_IS_CRITICAL need comments about
why they're there. It's never obvious why these things are being done.