From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-14.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42A28C433EF for ; Wed, 8 Sep 2021 08:44:27 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EBB2961166 for ; Wed, 8 Sep 2021 08:44:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org EBB2961166 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=pengutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:Cc: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=MLVlM2dN+THUOmnVzcdZxOQ4j8f8Ry9ToiRFUvFqvnM=; b=4vlmJ64MYByKCz aeplhxXguNtj/spAL289K93DM6GpbZMATIR1e3LLNm8uujhTfzlCo3f7Vrm/txBIZg3HeJ6f5+NP+ ruiKg3ru544m3MupOwW+44yBA0s/zGkoe46sQibSN47NCv6qNEvCnwn2VsbsExaEwkdwS867omYgl /hCLeB61icpCspGe9FG+ErtitnseWvMxlKyPAl9S40Hn6VJvFgeK+GfUfAWkDOUUURqAO4XNYe3d+ t9SWye36o1v8doNDOyrqwFl/w1SIg4X5/0lbBRBAGsOZY6dHXXDjQxakokHnpdHyS/auN3SKbUPil PCwK4OxvFDCsPvKFmmpQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mNt9v-006C8i-18; Wed, 08 Sep 2021 08:42:19 +0000 Received: from metis.ext.pengutronix.de ([2001:67c:670:201:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mNt9q-006C7g-43 for linux-arm-kernel@lists.infradead.org; Wed, 08 Sep 2021 08:42:15 +0000 Received: from gallifrey.ext.pengutronix.de ([2001:67c:670:201:5054:ff:fe8d:eefb] helo=[IPv6:::1]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mNt9k-0006us-96; Wed, 08 Sep 2021 10:42:08 +0200 Message-ID: <7ef982040749983045dd51cbd03c760293a56efe.camel@pengutronix.de> Subject: Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator dump From: Lucas Stach To: Richard Zhu , bhelgaas@google.com, lorenzo.pieralisi@arm.com Cc: linux-pci@vger.kernel.org, linux-imx@nxp.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kernel@pengutronix.de Date: Wed, 08 Sep 2021 10:42:07 +0200 In-Reply-To: <1631084366-24785-2-git-send-email-hongxing.zhu@nxp.com> References: <1631084366-24785-1-git-send-email-hongxing.zhu@nxp.com> <1631084366-24785-2-git-send-email-hongxing.zhu@nxp.com> User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) MIME-Version: 1.0 X-SA-Exim-Connect-IP: 2001:67c:670:201:5054:ff:fe8d:eefb X-SA-Exim-Mail-From: l.stach@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-20210908_014214_384596_D8F39A8E X-CRM114-Status: GOOD ( 29.89 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu: > Since there is error return check of the host_init callback, add error > check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > function. > > Because that i.MX PCIe doesn't support the hot-plug feature. To save > power consumption as much as possible, turn off the clocks and power > supplies when the PCIe PHY link is never came up in probe procedure. > > When PCIe link is never came up and vpcie regulator is present, there > would be following dump when try to put the regulator. > Disable this regulator to fix this dump when link is never came up. > > [ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up > [ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110 > [ 2.348160] ------------[ cut here ]------------ > [ 2.352778] WARNING: CPU: 3 PID: 119 at drivers/regulator/core.c:2256 _regulator_put.part.0+0x14c/0x158 > [ 2.362184] Modules linked in: > [ 2.365243] CPU: 3 PID: 119 Comm: kworker/u8:2 Not tainted 5.13.0-rc7-next-20210625-94710-ge4e92b2588a3 #10 > [ 2.374987] Hardware name: FSL i.MX8MM EVK board (DT) > [ 2.380040] Workqueue: events_unbound async_run_entry_fn > [ 2.385359] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--) > [ 2.391369] pc : _regulator_put.part.0+0x14c/0x158 > [ 2.396163] lr : regulator_put+0x34/0x48 > [ 2.400088] sp : ffff8000122ebb30 > [ 2.403400] x29: ffff8000122ebb30 x28: ffff800011be7000 x27: 0000000000000000 > [ 2.410546] x26: 0000000000000000 x25: 0000000000000000 x24: ffff00000025f2bc > [ 2.417689] x23: ffff00000025f2c0 x22: ffff00000025f010 x21: ffff8000122ebc18 > [ 2.424834] x20: ffff800011e3fa60 x19: ffff00000375fd80 x18: 0000000000000010 > [ 2.431979] x17: 000000040044ffff x16: 00400032b5503510 x15: 0000000000000108 > [ 2.439124] x14: ffff0000003cc938 x13: 00000000ffffffea x12: 0000000000000000 > [ 2.446267] x11: 0000000000000000 x10: ffff80001076ba88 x9 : ffff80001076a540 > [ 2.453411] x8 : ffff00000025f2c0 x7 : ffff0000001f4450 x6 : ffff000000176cd8 > [ 2.460556] x5 : ffff000003857880 x4 : 0000000000000000 x3 : ffff800011e3fe30 > [ 2.467700] x2 : ffff0000003cc4c0 x1 : 0000000000000000 x0 : 0000000000000001 > [ 2.474847] Call trace: > [ 2.477295] _regulator_put.part.0+0x14c/0x158 > [ 2.481742] regulator_put+0x34/0x48 > [ 2.485322] devm_regulator_release+0x10/0x18 > [ 2.489681] release_nodes+0x38/0x60 > [ 2.493262] devres_release_all+0x88/0xd0 > [ 2.497276] really_probe+0xd0/0x2e8 > [ 2.500858] __driver_probe_device+0x74/0xd8 > [ 2.505137] driver_probe_device+0x7c/0x108 > [ 2.509325] __device_attach_driver+0x8c/0xd0 > [ 2.513685] bus_for_each_drv+0x74/0xc0 > [ 2.517531] __device_attach_async_helper+0xb4/0xd8 > [ 2.522419] async_run_entry_fn+0x30/0x100 > [ 2.526521] process_one_work+0x19c/0x320 > [ 2.530532] worker_thread+0x48/0x418 > [ 2.534199] kthread+0x14c/0x158 > [ 2.537432] ret_from_fork+0x10/0x18 > [ 2.541013] ---[ end trace 3664ca4a50ce849b ]--- > > Signed-off-by: Richard Zhu > --- > drivers/pci/controller/dwc/pci-imx6.c | 28 +++++++++++++++++++-------- > 1 file changed, 20 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 0264432e4c4a..129928e42f84 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -144,6 +144,7 @@ struct imx6_pcie { > #define PHY_RX_OVRD_IN_LO_RX_PLL_EN BIT(3) > > static int imx6_pcie_clk_enable(struct imx6_pcie *imx6_pcie); > +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie); > > static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool exp_val) > { > @@ -485,24 +486,24 @@ static void imx7d_pcie_wait_for_phy_pll_lock(struct imx6_pcie *imx6_pcie) > dev_err(dev, "PCIe PLL lock timeout\n"); > } > > -static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > +static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > { > struct dw_pcie *pci = imx6_pcie->pci; > struct device *dev = pci->dev; > - int ret; > + int ret, err; Why do we need a separate variable for the error code here? Why not just use the existing ret? > > if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) { > ret = regulator_enable(imx6_pcie->vpcie); > if (ret) { > dev_err(dev, "failed to enable vpcie regulator: %d\n", > ret); > - return; > + return ret; > } > } > > - ret = imx6_pcie_clk_enable(imx6_pcie); > - if (ret) { > - dev_err(dev, "unable to enable pcie clocks\n"); > + err = imx6_pcie_clk_enable(imx6_pcie); > + if (err) { > + dev_err(dev, "unable to enable pcie clocks: %d\n", err); > goto err_clks; > } > > @@ -557,7 +558,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > break; > } > > - return; > + return 0; > > err_clks: > if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) { > @@ -566,6 +567,7 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > dev_err(dev, "failed to disable vpcie regulator: %d\n", > ret); > } > + return err; > } > > static void imx6_pcie_configure_type(struct imx6_pcie *imx6_pcie) > @@ -810,17 +812,27 @@ static int imx6_pcie_start_link(struct dw_pcie *pci) > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), > dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); > imx6_pcie_reset_phy(imx6_pcie); > + imx6_pcie_clk_disable(imx6_pcie); This is a separate fix for the clock enable counts, that isn't mentioned in the commit message. It seems like this patch fixes in fact 3 issues: error propagation from host_init, fix regulator reference handling, fix clk reference handling. Either split up the patch along those changes (if possible) or improve the commit message to explain those changes. Regards, Lucas > + if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) > + regulator_disable(imx6_pcie->vpcie); > return ret; > } > > static int imx6_pcie_host_init(struct pcie_port *pp) > { > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > + struct device *dev = pci->dev; > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pci); > + int ret; > > imx6_pcie_assert_core_reset(imx6_pcie); > imx6_pcie_init_phy(imx6_pcie); > - imx6_pcie_deassert_core_reset(imx6_pcie); > + ret = imx6_pcie_deassert_core_reset(imx6_pcie); > + if (ret < 0) { > + dev_err(dev, "pcie host init failed: %d.\n", ret); > + return ret; > + } > + > imx6_setup_phy_mpll(imx6_pcie); > > return 0; _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel