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

[Stephen Boyd]

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.