Coupled clk/reset

Coupled clk/reset

[Joel Stanley]

Hello!

I'm taking a stab at a clk and reset driver for the Aspeed SoCs. I've
used the Gemini driver for inspiration so far, which looks to be a
good fit for the clock gating and important clocks (those we need in
order to get the uart and timer source going).

One tricky bit is the datasheet specifies the following for enabling
an IP block ('engine'), whenever the engine is started from the
clocked stopped state:

 1. Enable engine reset
 2. Delay 100us
 3. Enable clock
 4. Delay 10ms
 5. Disable engine reset

How does this dance fit into the reset/clock framework? I need to show
that there is this dependency when enabling a clock.

Cheers,

Joel

Re: Coupled clk/reset

[Benjamin Herrenschmidt]

On Tue, 2017-07-18 at 19:53 +0930, Joel Stanley wrote:
> Hello!
> 
> I'm taking a stab at a clk and reset driver for the Aspeed SoCs. I've
> used the Gemini driver for inspiration so far, which looks to be a
> good fit for the clock gating and important clocks (those we need in
> order to get the uart and timer source going).
> 
> One tricky bit is the datasheet specifies the following for enabling
> an IP block ('engine'), whenever the engine is started from the
> clocked stopped state:
> 
>  1. Enable engine reset
>  2. Delay 100us
>  3. Enable clock
>  4. Delay 10ms
>  5. Disable engine reset
> 
> How does this dance fit into the reset/clock framework? I need to show
> that there is this dependency when enabling a clock.

My gut feeling is that I would have a single driver exposing both clock
and reset interfaces.

It keeps a "reset state" for each device.

The reset API just changes that internal (SW) state, and adjusts the HW
reset if and only if the clock is on.

When the clock is turned off, we force the reset, disable the clock.

When the clock is turned on, we force the reset if not already set,
enable the clock, then only lift the reset if the "user" reset is
not asserted.

That being said, I think for the aspeed SoC, the argument could be made
that we don't actually need a reset driver. We yet have to encounter a
single device that needs to control it's own reset line for anything
other than enabling/disabling its clock/IP block.

So I suspect sticking to just a clock API that does the underlying
reset manipulations as needed is sufficient and we can "upgrade" to the
above proposal if it ends up being needed (which I doubt).

Cheers,
Ben.

Re: Coupled clk/reset

[Philipp Zabel]

Hi Joel,

On Tue, 2017-07-18 at 19:53 +0930, Joel Stanley wrote:
> Hello!
> 
> I'm taking a stab at a clk and reset driver for the Aspeed SoCs. I've
> used the Gemini driver for inspiration so far, which looks to be a
> good fit for the clock gating and important clocks (those we need in
> order to get the uart and timer source going).
> 
> One tricky bit is the datasheet specifies the following for enabling
> an IP block ('engine'), whenever the engine is started from the
> clocked stopped state:
> 
>  1. Enable engine reset
>  2. Delay 100us
>  3. Enable clock
>  4. Delay 10ms
>  5. Disable engine reset
>
> How does this dance fit into the reset/clock framework? I need to show
> that there is this dependency when enabling a clock.

If this is some special treatment for a single IP block, I'd just open
code this in the driver:
	reset_control_assert(rstc);
	usleep_range(100, 200);
	clk_prepare_enable(clk);
	usleep_range(10000, 20000);
	reset_control_deassert(rstc);

If this is a common pattern needed for multiple clocks, you could
investigate hiding this in the clock driver and not export the resets to
the 'engine' driver at all.

regards
Philipp

Re: Coupled clk/reset

[Philipp Zabel]

On Tue, 2017-07-18 at 20:56 +1000, Benjamin Herrenschmidt wrote:
[...]
> My gut feeling is that I would have a single driver exposing both clock
> and reset interfaces.

Agreed.

> It keeps a "reset state" for each device.
> 
> The reset API just changes that internal (SW) state, and adjusts the HW
> reset if and only if the clock is on.
> 
> When the clock is turned off, we force the reset, disable the clock.
> 
> When the clock is turned on, we force the reset if not already set,
> enable the clock, then only lift the reset if the "user" reset is
> not asserted.

Please let's not do this. If a reset line is controlled by the clocks
like this, I'd rather not have it exported via the reset API at the same
time.
Both shared and exclusive reset control API are incompatible with
another controlling instance forcing the reset line into asserted state:
the exclusive reset API promises full control over the reset line, and
the shared reset API guarantees deassertion while requested (just like
the clk API can't be used to force-disable a clock).

> That being said, I think for the aspeed SoC, the argument could be made
> that we don't actually need a reset driver. We yet have to encounter a
> single device that needs to control it's own reset line for anything
> other than enabling/disabling its clock/IP block.
> 
> So I suspect sticking to just a clock API that does the underlying
> reset manipulations as needed is sufficient and we can "upgrade" to the
> above proposal if it ends up being needed (which I doubt).

I agree with hiding the whole mechanism in the clock driver. If that
can't be done in the clock driver, I would prefer to give control to the
IP block drivers instead.

regards
Philipp

Re: Coupled clk/reset

[Benjamin Herrenschmidt]

On Tue, 2017-07-18 at 16:06 +0200, Philipp Zabel wrote:
> If this is some special treatment for a single IP block, I'd just open
> code this in the driver:
>         reset_control_assert(rstc);
>         usleep_range(100, 200);
>         clk_prepare_enable(clk);
>         usleep_range(10000, 20000);
>         reset_control_deassert(rstc);
> 
> If this is a common pattern needed for multiple clocks, you could
> investigate hiding this in the clock driver and not export the resets to
> the 'engine' driver at all.

Right, it's a common pattern for all the IP blocks in that SoC. I agree
with your proposal, it would keep things simpler.

Cheers,
Ben.

Re: Coupled clk/reset

[Joel Stanley]

On Wed, Jul 19, 2017 at 6:44 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2017-07-18 at 16:06 +0200, Philipp Zabel wrote:
>> If this is some special treatment for a single IP block, I'd just open
>> code this in the driver:
>>         reset_control_assert(rstc);
>>         usleep_range(100, 200);
>>         clk_prepare_enable(clk);
>>         usleep_range(10000, 20000);
>>         reset_control_deassert(rstc);
>>
>> If this is a common pattern needed for multiple clocks, you could
>> investigate hiding this in the clock driver and not export the resets to
>> the 'engine' driver at all.
>
> Right, it's a common pattern for all the IP blocks in that SoC. I agree
> with your proposal, it would keep things simpler.

Thanks Philipp and Ben. I'll go ahead with a clock driver that does
the right things, and drop the reset controller parts.

Cheers,

Joel

Re: Coupled clk/reset

[Peter De Schrijver]

On Tue, Jul 18, 2017 at 07:53:09PM +0930, Joel Stanley wrote:
> Hello!
> 
> I'm taking a stab at a clk and reset driver for the Aspeed SoCs. I've
> used the Gemini driver for inspiration so far, which looks to be a
> good fit for the clock gating and important clocks (those we need in
> order to get the uart and timer source going).
> 
> One tricky bit is the datasheet specifies the following for enabling
> an IP block ('engine'), whenever the engine is started from the
> clocked stopped state:
> 
>  1. Enable engine reset
>  2. Delay 100us
>  3. Enable clock
>  4. Delay 10ms
>  5. Disable engine reset
> 

This is a pretty standard sequence. The reset is synchronous, so the module
clock needs to be running for the reset toggling to take effect. And it takes
some time for the reset propagate through all the logic, hence the second
delay, although 10ms seems large to me.

Peter.