On 2018-10-19 16:09, Taniya Das wrote:
On 10/17/2018 7:50 PM, Stephen Boyd wrote:
Quoting Taniya Das (2018-10-17 05:04:10)
On 10/17/2018 5:07 PM, Taniya Das wrote:
Hello Stephen,
On 10/12/2018 11:05 PM, Stephen Boyd wrote:
Quoting Taniya Das (2018-10-09 23:12:27)
On 10/10/2018 2:22 AM, Stephen Boyd wrote:
Quoting Taniya Das (2018-10-09 10:26:38)
Hello Stephen,
On 10/8/2018 8:14 AM, Stephen Boyd wrote:
Quoting Taniya Das (2018-10-04 05:02:26)
Add support for the lpass clock controller found on SDM845
based
devices.
This would allow lpass peripheral loader drivers to control
the
clocks to
bring the subsystem out of reset.
LPASS clocks present on the global clock controller would be
registered
with the clock framework based on the device tree flag. Also
do
not gate
these clocks if they are left unused.
Why not gate them? This statement states what the code is
doing,
not why
it's doing it which is the more crucial information that should
be
described in the commit text. Also, please add a comment about
it
to the
code next to the flag.
I am concerned that it doesn't make any sense though, so
probably it
shouldn't be marked as CLK_IGNORE_UNUSED and it's papering over
some
other larger bug that needs to be fixed.
It does not have any bug, it is just that to access these lpass
registers we would need the GCC lpass registers to be enabled. I
would
update the same in the commit text.
During clock late_init these clocks should not be accessed to
check
the
clock status as they would result in unclocked access. The
client
would
request these clocks in the correct order and it would not have
any
issue.
That seems like the bug right there. If the LPASS registers can't
be
accessed unless the clks in GCC are enabled then this driver
needs to
turn the clks on before reading/writing registers. Marking the
clks as
ignore unused is skipping around the real problem.
If the driver requests for the clocks they would maintain the
order. But
if the clock late init call is invoked before the driver requests,
there
is no way I could manage this dependency, that is the only reason
to
mark them unused.
Which driver are we talking about here? The lpass clk driver?
Presumably
the lpass clk driver would request the GCC clks and turn them on in
probe and then register any lpass clks. If the lpass clk driver
probes
bfeore late init, then the gcc clks will be enabled and everything
works, and if the lpass clk driver probes after late init then the
clks
that can't be touched without gcc clks enabled won't be registered,
and
then they won't be touched. What goes wrong?
Okay, sure, I will take the GCC clock handles and then
enable/disable
them accordingly.
I missed earlier, so here is what you suggest
gcc_probe --> GCC LPASS clocks registered.
lpass_probe --> clk_get on gcc_lpass_clocks/ clk_prepare_enable -->
register the lpass clocks --> clk_disable_unprepare gcc_lpass_clocks.
Why did the gcc_lpass_clocks get turned off? Shouldn't they just stay
enabled all the time?
I don't think they are kept enabled all the time.
But the problem is not during the above. It is the below
static void clk_disable_unused_subtree(struct clk_core *core)
{
....
if (clk_core_is_enabled(core)) { --> This access fails.
....
}
You may need to add some prepare_ops to turn on clks needed to
read/write lpass registers. Or you can look into using some sort of
genpd to enable required clks when these clks are enabled or disabled.
But I suspect it would be easier to just leave the clks in GCC for
lpass
always enabled and not worry about the complicated genpd things.
I need to check if keeping them enabled/marking them CRITICAL could
have an impact on the reset of the subsystem.
I have checked internally with the teams and the GCC LPASS clocks could
be left enabled.
Would submit a patch keeping them CRITICAL.
