clk: gemini: Fix reset regression

Message ID 20170711122601.17745-1-linus.walleij@linaro.org
State Accepted
Commit f905293d655cbb8be833261ef390aaba71b44307
Headers show

Commit Message

Linus Walleij July 11, 2017, 12:26 p.m.
commit e2860e1f62f2 ("serial: 8250_of: Add reset support")
introduced reset support for the 8250_of driver.

However it unconditionally uses the assert/deassert pair to
deassert reset on the device at probe and assert it at
remove. This does not work with systems that have a
self-deasserting reset controller, such as Gemini, that
recently added a reset controller.

As a result, the console will not probe on the Gemini with
this message:

Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled
of_serial: probe of 42000000.serial failed with error -524

This (-ENOTSUPP) is the error code returned by the
deassert() operation on self-deasserting reset controllers.

To work around this, implement dummy .assert() and
.deassert() operations in the Gemini combined clock and
reset controller. This fixes the issue on this system.

Cc: Joel Stanley <joel@jms.id.au>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-serial@vger.kernel.org
Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
This is the solution suggested by Philipp, I think.
---
 drivers/clk/clk-gemini.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

-- 
2.9.4

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Philipp Zabel July 12, 2017, 3:51 p.m. | #1
Hi Linus,

On Tue, 2017-07-11 at 14:26 +0200, Linus Walleij wrote:
> commit e2860e1f62f2 ("serial: 8250_of: Add reset support")

> introduced reset support for the 8250_of driver.

> 

> However it unconditionally uses the assert/deassert pair to

> deassert reset on the device at probe and assert it at

> remove. This does not work with systems that have a

> self-deasserting reset controller, such as Gemini, that

> recently added a reset controller.

> 

> As a result, the console will not probe on the Gemini with

> this message:

> 

> Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled

> of_serial: probe of 42000000.serial failed with error -524

> 

> This (-ENOTSUPP) is the error code returned by the

> deassert() operation on self-deasserting reset controllers.

> 

> To work around this, implement dummy .assert() and

> .deassert() operations in the Gemini combined clock and

> reset controller. This fixes the issue on this system.

>

> Cc: Joel Stanley <joel@jms.id.au>

> Cc: Philipp Zabel <p.zabel@pengutronix.de>

> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> Cc: linux-serial@vger.kernel.org

> Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> This is the solution suggested by Philipp, I think.


It is what I suggested, yes, but now that I see it before me, I don't
think this is the proper solution either. Reason below:

> ---

>  drivers/clk/clk-gemini.c | 14 ++++++++++++++

>  1 file changed, 14 insertions(+)

> 

> diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c

> index c391a49aaaff..b4cf2f699a21 100644

> --- a/drivers/clk/clk-gemini.c

> +++ b/drivers/clk/clk-gemini.c

> @@ -237,6 +237,18 @@ static int gemini_reset(struct reset_controller_dev *rcdev,

>  			    BIT(GEMINI_RESET_CPU1) | BIT(id));

>  }

>  

> +static int gemini_reset_assert(struct reset_controller_dev *rcdev,

> +			       unsigned long id)

> +{

> +	return 0;


This is valid behaviour for shared reset controls, as sharing users
don't mind whether the reset line is actually asserted after this call,
they just allow it.

For an exclusive reset control this should return an error though, as
the caller would expect the reset line to be asserted after this call.
Unfortunately the core does not provide information whether the reset
control is shared or exclusive to the reset drivers, and it could be
argued that the drivers shouldn't have to care. I suppose I'll have to
handle this in the core, after all. What do you think of the attached
patch?

Otherwise, as a regression fix, I think this would be ok. There isn't
going to be any driver on the Gemini platform that requests an exclusive
reset control and then calls reset_control_assert, expecting the reset
line to stay asserted.

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>


> +}

> +

> +static int gemini_reset_deassert(struct reset_controller_dev *rcdev,

> +				 unsigned long id)

> +{

> +	return 0;


This is valid behaviour for a self-deasserting controller with the reset
lines initially deasserted for both shared and exclusive reset controls:
after this call the reset line is guaranteed to be deasserted.

regards
Philipp

----------8<----------
From fab3a9a697e9797ba1c24874d7c43c09dd812e77 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>

Date: Wed, 12 Jul 2017 17:29:28 +0200
Subject: [PATCH] reset: make (de)assert report succeess for self-deasserting
 reset drivers

By now there are drivers using shared reset controls and (de)assert
calls on platforms with self-deasserting reset lines and thus reset
drivers that do not implement .assert() and .deassert().
As long as the initial state of the reset line is deasserted, there
is no reason for a reset_control_assert call to return an error for
shared reset controls, or for a reset_control_deassert call to return
an error for either shared or exclusive reset controls: after a call
to reset_control_deassert the reset line is guaranteed to be deasserted,
and after a call to reset_control_assert it is valid for the reset
line to stay deasserted for shared reset controls.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

---
 drivers/reset/core.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

-- 
2.11.0
---------->8----------

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.htmldiff --git a/drivers/reset/core.c b/drivers/reset/core.c
index cd739d2fa1603..6c182c173f9b4 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -201,9 +201,6 @@ int reset_control_assert(struct reset_control *rstc)
 	if (WARN_ON(IS_ERR(rstc)))
 		return -EINVAL;
 
-	if (!rstc->rcdev->ops->assert)
-		return -ENOTSUPP;
-
 	if (rstc->shared) {
 		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
 			return -EINVAL;
@@ -213,6 +210,21 @@ int reset_control_assert(struct reset_control *rstc)
 
 		if (atomic_dec_return(&rstc->deassert_count) != 0)
 			return 0;
+
+		/*
+		 * Shared reset controls allow the reset line to be in any state
+		 * after this call, so doing nothing is a valid option.
+		 */
+		if (!rstc->rcdev->ops->assert)
+			return 0;
+	} else {
+		/*
+		 * If the reset controller does not implement .assert(), there
+		 * is no way to guarantee that the reset line is asserted after
+		 * this call.
+		 */
+		if (!rstc->rcdev->ops->assert)
+			return -ENOTSUPP;
 	}
 
 	return rstc->rcdev->ops->assert(rstc->rcdev, rstc->id);
@@ -239,9 +251,6 @@ int reset_control_deassert(struct reset_control *rstc)
 	if (WARN_ON(IS_ERR(rstc)))
 		return -EINVAL;
 
-	if (!rstc->rcdev->ops->deassert)
-		return -ENOTSUPP;
-
 	if (rstc->shared) {
 		if (WARN_ON(atomic_read(&rstc->triggered_count) != 0))
 			return -EINVAL;
@@ -250,6 +259,16 @@ int reset_control_deassert(struct reset_control *rstc)
 			return 0;
 	}
 
+	/*
+	 * If the reset controller does not implement .deassert(), we assume
+	 * that it handles self-deasserting reset lines via .reset(). In that
+	 * case, the reset lines are deasserted by default. If that is not the
+	 * case, the reset controller driver should implement .deassert() and
+	 * return -ENOTSUPP.
+	 */
+	if (!rstc->rcdev->ops->deassert)
+		return 0;
+
 	return rstc->rcdev->ops->deassert(rstc->rcdev, rstc->id);
 }
 EXPORT_SYMBOL_GPL(reset_control_deassert);

Stephen Boyd July 12, 2017, 11:08 p.m. | #2
On 07/12, Philipp Zabel wrote:
> Hi Linus,

> 

> On Tue, 2017-07-11 at 14:26 +0200, Linus Walleij wrote:

> > commit e2860e1f62f2 ("serial: 8250_of: Add reset support")

> > introduced reset support for the 8250_of driver.

> > 

> > However it unconditionally uses the assert/deassert pair to

> > deassert reset on the device at probe and assert it at

> > remove. This does not work with systems that have a

> > self-deasserting reset controller, such as Gemini, that

> > recently added a reset controller.

> > 

> > As a result, the console will not probe on the Gemini with

> > this message:

> > 

> > Serial: 8250/16550 driver, 1 ports, IRQ sharing disabled

> > of_serial: probe of 42000000.serial failed with error -524

> > 

> > This (-ENOTSUPP) is the error code returned by the

> > deassert() operation on self-deasserting reset controllers.

> > 

> > To work around this, implement dummy .assert() and

> > .deassert() operations in the Gemini combined clock and

> > reset controller. This fixes the issue on this system.

> >

> > Cc: Joel Stanley <joel@jms.id.au>

> > Cc: Philipp Zabel <p.zabel@pengutronix.de>

> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

> > Cc: linux-serial@vger.kernel.org

> > Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support")

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> > ---

> > This is the solution suggested by Philipp, I think.

> 

> It is what I suggested, yes, but now that I see it before me, I don't

> think this is the proper solution either. Reason below:

> 

> > ---

> >  drivers/clk/clk-gemini.c | 14 ++++++++++++++

> >  1 file changed, 14 insertions(+)

> > 

> > diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c

> > index c391a49aaaff..b4cf2f699a21 100644

> > --- a/drivers/clk/clk-gemini.c

> > +++ b/drivers/clk/clk-gemini.c

> > @@ -237,6 +237,18 @@ static int gemini_reset(struct reset_controller_dev *rcdev,

> >  			    BIT(GEMINI_RESET_CPU1) | BIT(id));

> >  }

> >  

> > +static int gemini_reset_assert(struct reset_controller_dev *rcdev,

> > +			       unsigned long id)

> > +{

> > +	return 0;

> 

> This is valid behaviour for shared reset controls, as sharing users

> don't mind whether the reset line is actually asserted after this call,

> they just allow it.

> 

> For an exclusive reset control this should return an error though, as

> the caller would expect the reset line to be asserted after this call.

> Unfortunately the core does not provide information whether the reset

> control is shared or exclusive to the reset drivers, and it could be

> argued that the drivers shouldn't have to care. I suppose I'll have to

> handle this in the core, after all. What do you think of the attached

> patch?

> 

> Otherwise, as a regression fix, I think this would be ok. There isn't

> going to be any driver on the Gemini platform that requests an exclusive

> reset control and then calls reset_control_assert, expecting the reset

> line to stay asserted.

> 

> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> 


I'll queue this up for clk-fixes once -rc1 is out.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 18, 2017, 9:49 a.m. | #3
On Wed, Jul 12, 2017 at 5:51 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> From fab3a9a697e9797ba1c24874d7c43c09dd812e77 Mon Sep 17 00:00:00 2001

> From: Philipp Zabel <p.zabel@pengutronix.de>

> Date: Wed, 12 Jul 2017 17:29:28 +0200

> Subject: [PATCH] reset: make (de)assert report succeess for self-deasserting

>  reset drivers

>

> By now there are drivers using shared reset controls and (de)assert

> calls on platforms with self-deasserting reset lines and thus reset

> drivers that do not implement .assert() and .deassert().

> As long as the initial state of the reset line is deasserted, there

> is no reason for a reset_control_assert call to return an error for

> shared reset controls, or for a reset_control_deassert call to return

> an error for either shared or exclusive reset controls: after a call

> to reset_control_deassert the reset line is guaranteed to be deasserted,

> and after a call to reset_control_assert it is valid for the reset

> line to stay deasserted for shared reset controls.

>

> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>


This patch makes all kind of sense, and I follow your
reasoning.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>


In the back of my head I was thinking that the deassert/assert
pair matches a certain SoC driver design pattern I have seen
around:

When a driver use an IP block it enables the clock and takes
the block out of reset.

When it stops using it, it asserts reset and disables the clock.

This is not entirely self-evident, for example why is reset asserted
across say insmod/rmmod/insmod. Just disabling the clock would
be OK, I guess, in most cases. It is just one of those "dances"
that developers do, as if to clear the desk or something.

But I guess there must be cases where doing things in another way
creates problems or power leaks.

I would surely like to understand, from a silicon perspective, why
drivers are so often written like this. I could think of things like
little automata and gates inside the silicon that need to be reset
to minimize off-power consumption but I have no clue if it is
really so.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter De Schrijver July 19, 2017, 8 a.m. | #4
On Tue, Jul 18, 2017 at 11:49:44AM +0200, Linus Walleij wrote:
> On Wed, Jul 12, 2017 at 5:51 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> 

> > From fab3a9a697e9797ba1c24874d7c43c09dd812e77 Mon Sep 17 00:00:00 2001

> > From: Philipp Zabel <p.zabel@pengutronix.de>

> > Date: Wed, 12 Jul 2017 17:29:28 +0200

> > Subject: [PATCH] reset: make (de)assert report succeess for self-deasserting

> >  reset drivers

> >

> > By now there are drivers using shared reset controls and (de)assert

> > calls on platforms with self-deasserting reset lines and thus reset

> > drivers that do not implement .assert() and .deassert().

> > As long as the initial state of the reset line is deasserted, there

> > is no reason for a reset_control_assert call to return an error for

> > shared reset controls, or for a reset_control_deassert call to return

> > an error for either shared or exclusive reset controls: after a call

> > to reset_control_deassert the reset line is guaranteed to be deasserted,

> > and after a call to reset_control_assert it is valid for the reset

> > line to stay deasserted for shared reset controls.

> >

> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

> 

> This patch makes all kind of sense, and I follow your

> reasoning.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> 

> In the back of my head I was thinking that the deassert/assert

> pair matches a certain SoC driver design pattern I have seen

> around:

> 

> When a driver use an IP block it enables the clock and takes

> the block out of reset.

> 

> When it stops using it, it asserts reset and disables the clock.

> 

> This is not entirely self-evident, for example why is reset asserted

> across say insmod/rmmod/insmod. Just disabling the clock would

> be OK, I guess, in most cases. It is just one of those "dances"

> that developers do, as if to clear the desk or something.

> 

> But I guess there must be cases where doing things in another way

> creates problems or power leaks.

> 

> I would surely like to understand, from a silicon perspective, why

> drivers are so often written like this. I could think of things like

> little automata and gates inside the silicon that need to be reset

> to minimize off-power consumption but I have no clue if it is

> really so.


At least doing it this way guarantees the hardware is in its initial state
before the driver starts doing anything with it. Otherwise you would have
to ensure the hw init sequence in the driver works for any hw state. That
seems more complicated to me than just making sure it works correctly when
the hw is in its initial stat, especially when there are DMA engines involved.

Peter.

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/clk/clk-gemini.c b/drivers/clk/clk-gemini.c
index c391a49aaaff..b4cf2f699a21 100644
--- a/drivers/clk/clk-gemini.c
+++ b/drivers/clk/clk-gemini.c
@@ -237,6 +237,18 @@  static int gemini_reset(struct reset_controller_dev *rcdev,
 			    BIT(GEMINI_RESET_CPU1) | BIT(id));
 }
 
+static int gemini_reset_assert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	return 0;
+}
+
+static int gemini_reset_deassert(struct reset_controller_dev *rcdev,
+				 unsigned long id)
+{
+	return 0;
+}
+
 static int gemini_reset_status(struct reset_controller_dev *rcdev,
 			     unsigned long id)
 {
@@ -253,6 +265,8 @@  static int gemini_reset_status(struct reset_controller_dev *rcdev,
 
 static const struct reset_control_ops gemini_reset_ops = {
 	.reset = gemini_reset,
+	.assert = gemini_reset_assert,
+	.deassert = gemini_reset_deassert,
 	.status = gemini_reset_status,
 };