Re: [PATCH v6 1/3] clk: qcom: Configure the RCGs to a safe source as needed

From: Amit Nischal
Date: Mon May 07 2018 - 06:33:41 EST

On 2018-05-05 08:54, Stephen Boyd wrote:
Quoting Amit Nischal (2018-05-03 04:57:37)
On 2018-05-02 13:15, Stephen Boyd wrote:
> Quoting Amit Nischal (2018-04-30 09:20:08)
>
>> +}
>> +
>> +static void clk_rcg2_shared_disable(struct clk_hw *hw)
>> +{
>> + struct clk_rcg2 *rcg = to_clk_rcg2(hw);
>> + struct freq_tbl safe_src_tbl = { 0 };
>> +
>> + /*
>> + * Park the RCG at a safe configuration - sourced off from
>> safe source.
>> + * Force enable and disable the RCG while configuring it to
>> safeguard
>> + * against any update signal coming from the downstream clock.
>> + * The current parent is still prepared and enabled at this
>> point, and
>> + * the safe source is always on while application processor
>> subsystem
>> + * is online. Therefore, the RCG can safely switch its parent.
>> + */
>> + safe_src_tbl.src = rcg->safe_src_index;
>> + clk_rcg2_shared_force_enable_clear(hw, &safe_src_tbl);
>
> This should then re-dirty the config register to have the software
> frequency settings that existed in the hardware when disable was
> called.
> Given that MND shouldn't be changed here, this should be as simple as
> saving away the CFG register, forcing it to XO speed by changing the
> src
> and disabling dual edge in the CFG register (please don't call
> force_enable_clear with some frequency pointer, just do this inline
> here), and then rewriting the cfg register with the "real" settings for
> whatever frequency it's supposed to run at and then returning from this
> function.
>
> I guess we have to do a read cfg from hardware, write cfg, hit update
> config, and then write cfg again each time we disable. For enable, we
> just do an update config (if it's dirty?) inside of a force
> enable/disable pair. And set_rate doesn't really change except it
> either
> does or doesn't hit the update config bit if the clk is enabled or
> disabled respectively.
>

We have done the below changes suggested by you and that logic seems to
be
working fine. But we have one concern about leaving the RCG registers in
dirty state and would like to have a little bit modification as
explained
below:

Suggested Logic:
1. set_rate()--> Update CFG, M, N and D registers and don't hit the
update
bit if clock is disabled - call new
__clk_rcg2_configure().
Above will make the CFG register as dirty.

2. _disable()--> 2.1 - Store the CFG register configuration in a
variable.
2.2 - Move to the safe source (XO) and hit the update
bit.
It will only touch the CFG register and M, N, D
register values will remain as it was.
2.3 - Write back the stored CFG value done in step #2.1
This will again redirty the CFG register.

3. _enable()--> Just hit the update bit as the configuration write will
be taken care in the steps #1 and #2.

It would be great if we don't redirty the CFG register and leave the RCG
CFG register to at safe source(XO) in disable() path.

This would help us to debug the issues where device crashes and we want
to dump the RCG registers to know whether from software, we have
actually
moved to safe source or not. Otherwise, we would get the dirty register
values in crash dumps.

So instead of writing back the stored CFG(corresponding to real rate
settings) in disable path, we want to restore the stored CFG in enable
path and then hit the update bit.
CFG configuration store can happen at two places - set_rate() and
disable()
path and above logic will be modified as below:

1. set_rate()--> 1.1 - Update CFG, M, N and D registers and don't hit
the
update bit if clock is disabled.
1.2 - Store CFG register value in 'current_cfg' member
of 'rcg2' structure.

2. _disable()--> 2.1 - Store the CFG register value in 'current_cfg'
before
switching to the safe source (XO).
2.2 - Move to the safe source (XO) and hit the update
bit.
Now RCG configuration wil not be dirty.

3. _enable()--> 3.1 - Check for 'current_cfg' value and if 0 then
return.
This would catch the below one time condition:
- when clk_enable() gets call without set_rate().

We want clk_enable() to work without set_rate() though. So returning 0
if the value is 0 is wrong.

3.2 - Write the CFG value from 'current_cfg' to CFG
register.
3.2 - Hit the update bit as we have already written the
latest
configuration in step #3.2.
3.3 - Clear the 'current_cfg' value.

We feel above logic will work as expected and in this way, we don't have
CFG
registers in dirty state when clock is disabled.
Could you please inform us your thoughts about above implementation and
based
on that I will send the required changes.


If you want to save away current_cfg in a memory location so you know
what it was before it was dirty, then perhaps that needs to be a debug
patch that you stack on top when debugging. In the end, it would just be
saving the state of the frequency setting that we have in software
though, and that would be the latest rate of the clk we have configured
the clk for. There aren't any mux switches going on when the clk is off,
so you're saying you want to know the rate of the clk that the kernel
set when we turned the clk off, which we already have with the clk rate.
I'm confused.

Thanks for the suggestions. We will implement the new ops with the below
logic which you suggested earlier i.e. re-dirtying the RCG in disable() path
with real CFG settings and in enable() path, only hit the update bit.

1. set_rate()--> Update CFG, M, N and D registers and don't hit the
update bit if clock is disabled - call new
__clk_rcg2_configure().
Above will make the CFG register as dirty.

2. _disable()--> 2.1 - Store the CFG register configuration in a
variable.
2.2 - Move to the safe source (XO) and hit the update
bit. It will only touch the CFG register and M, N, D
register values will remain as it was.
2.3 - Write back the stored CFG value done in step #2.1
This will again redirty the CFG register.

3. _enable()--> Just hit the update bit as the configuration write will
be taken care in the steps #1 and #2.