From patchwork Wed Jul 12 15:51:22 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Philipp Zabel X-Patchwork-Id: 9837057 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id CC9CA602D8 for ; Wed, 12 Jul 2017 15:51:56 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BCE3B2860E for ; Wed, 12 Jul 2017 15:51:56 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B0D2428623; Wed, 12 Jul 2017 15:51:56 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.9 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID autolearn=unavailable version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id EC4AE2862C for ; Wed, 12 Jul 2017 15:51:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Mime-Version:References:In-Reply-To: Date:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=qAHkqqgJOSAtolLSLz4ZSTHET2pzHd982fCZrIAwkzQ=; b=rf0d5+dCDQQaSm a+JRGHuZtFG3Z0hzeWOPCIzMVvkBYhW4eu67OGN5O1qz2LmvPxjmWPRZOhsTJ7CNa+YNLMo2EiOJY IjJBqi9dwVGjnkim0IcPKgIAPIzSMZbi1/3OTC/i2KR9eWrkUmd2TfJANMoFO48N8WdmjHAI404Pt FvTnL2fm2kRCGSAG0dHhcva1IEPrtqxiMmcfgkBOxwHMvGFV7V1trQjSfMC1sIv7d70EdMIgLNOIJ SUGBd12AL8bFdMql70KltTKfVZMdfkg4datBZLZ/re1I1g0Jr9lKHsXsJp7iWT7cMnGluK2BNTC9S uxmumgQGoGIJnbGxOnog==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dVJvc-0003M1-Ip; Wed, 12 Jul 2017 15:51:52 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dVJvY-0003LB-Dg for linux-arm-kernel@lists.infradead.org; Wed, 12 Jul 2017 15:51:50 +0000 Received: from lupine.hi.pengutronix.de ([2001:67c:670:100:3ad5:47ff:feaf:1a17] helo=lupine) by metis.ext.pengutronix.de with esmtp (Exim 4.84_2) (envelope-from ) id 1dVJv8-0001EM-QP; Wed, 12 Jul 2017 17:51:22 +0200 Message-ID: <1499874682.6374.57.camel@pengutronix.de> Subject: Re: [PATCH] clk: gemini: Fix reset regression From: Philipp Zabel To: Linus Walleij Date: Wed, 12 Jul 2017 17:51:22 +0200 In-Reply-To: <20170711122601.17745-1-linus.walleij@linaro.org> References: <20170711122601.17745-1-linus.walleij@linaro.org> X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-arm-kernel@lists.infradead.org X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170712_085148_686106_5FA7A6BD X-CRM114-Status: GOOD ( 31.00 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Florian Fainelli , Paulius Zaleckas , Greg Kroah-Hartman , Michael Turquette , Stephen Boyd , Joel Stanley , linux-serial@vger.kernel.org, Janos Laube , Hans Ulli Kroll , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP 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 > Cc: Philipp Zabel > Cc: Greg Kroah-Hartman > Cc: linux-serial@vger.kernel.org > Fixes: e2860e1f62f2 ("serial: 8250_of: Add reset support") > Signed-off-by: Linus Walleij > --- > 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 > +} > + > +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 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 Reviewed-by: Linus Walleij --- drivers/reset/core.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --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);