Message-ID: <70b26f71-cccb-0730-6a50-7876222e228c@redhat.com>
Date:	Thu, 2 Jun 2016 12:50:06 +0200
From:	Hans de Goede <hdegoede@...hat.com>
To:	Lee Jones <lee.jones@...aro.org>, p.zabel@...gutronix.de
Cc:	linux-kernel@...r.kernel.org
Subject: Re: Shared Resets

Hi,

On 02-06-16 12:12, Lee Jones wrote:
> Philipp, Hans,
>
> After rebasing to the most recent upstream kernel tag (v4.6-rc1),

I think you mean 4.7-rc1, right ?

> I witnessed some issues where a couple of USB PHY drivers where having
> trouble requesting their reset lines.  After some poking around, I
> stumbled across Hans' shared reset patch.  It looks reasonable at
> first, but when I came to make the necessary driver changes, I
> realised that your effectively insisting that drivers have platform
> specific knowledge.
>
> The way I see it, drivers should request, deassert and assert their
> reset lines.  Just as clock users, request, enable and disable their
> clocks.

Right, but (read below), so if this is all you want then all you need
to do is change the code requesting the reset lines to use the
_shared postfixed variant of the reset_control_get function you
were already using in *all* users of that reset.

If you are already accidentally sharing resets you will actually
want my changes, as they ensure that the reset does not get
asserted until the last driver is done with it, where as before
the first driver to (re-)assert the reset in its cleanup function
would cause it to be asserted for all attached hardware blocks.

 > Whether the reset lines (or clocks) are left on after an
> assert (disable) because there are other users, should be of no
> concern to the consumer, right?

For most users yes.

> I would like to see (and am happy to author) a different approach,
> where knowledge of whether a reset line is shared or not is contained
> in the framework, as it is in the clock case.

The problem is that this is not possible because in some cases
the driver wants to actually be sure that reset_control_assert
actually asserts the reset, because the reset is used for error
handling and is needed to get the hw back into a sane state.

My patch-set actually started with your approach, but that got
nacked because of this and we already have drivers which rely
on this, e.g. the tegra usb phy driver needs to really _reset_
the block. rather then just treating reset as another resource
which needs to be enabled when in use and then released for
power management reasons.

Quoting from the docbook comments with my changes in it:

/**
  * reset_control_assert - asserts the reset line
  * @rstc: reset controller
  *
  * Calling this on an exclusive reset controller guarantees that the reset
  * will be asserted. When called on a shared reset controller the line may
  * still be deasserted, as long as other users keep it so.
  *
  * For shared reset controls a driver cannot expect the hw's registers and
  * internal state to be reset, but must be prepared for this to happen.
  */

And:

/**
  * reset_control_reset - reset the controlled device
  * @rstc: reset controller
  *
  * Calling this on a shared reset controller is an error.
  */

So we really have 2 different use-cases of reset lines here, one is
"take this block out of low-power mode", the other is "reset this block into
a sane hw state".

The one you're describing were resets work just like clocks, is the
"take this block out of low-power mode" one and is actually offered by
the new shared-reset functionality:

/**
  * reset_control_get_shared - Lookup and obtain a shared reference to a
  *                            reset controller.
  * @dev: device to be reset by the controller
  * @id: reset line name
  *
  * Returns a struct reset_control or IS_ERR() condition containing errno.
  * This function is intended for use with reset-controls which are shared
  * between hardware-blocks.
  *
  * When a reset-control is shared, the behavior of reset_control_assert /
  * deassert is changed, the reset-core will keep track of a deassert_count
  * and only (re-)assert the reset after reset_control_assert has been called
  * as many times as reset_control_deassert was called. Also see the remark
  * about shared reset-controls in the reset_control_assert docs.
  *
  * Calling reset_control_assert without first calling reset_control_deassert
  * is not allowed on a shared reset control. Calling reset_control_reset is
  * also not allowed on a shared reset control.
  */


TL;DR: If you want resets to work as clock, use reset_control_get_shared
(or a variant of it) and then things will work exactly as you want.


Regards,

Hans




>
> What are your thoughts?
>
> Kind regards,
> Lee
>

Powered by blists - more mailing lists