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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0826C433F5 for ; Tue, 26 Oct 2021 16:38:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 86EF46108B for ; Tue, 26 Oct 2021 16:38:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236037AbhJZQlL (ORCPT ); Tue, 26 Oct 2021 12:41:11 -0400 Received: from mail.kernel.org ([198.145.29.99]:47292 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232718AbhJZQlJ (ORCPT ); Tue, 26 Oct 2021 12:41:09 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8E62260EC0; Tue, 26 Oct 2021 16:38:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1635266325; bh=M6fXJMrTGa8IExJqf4YZcO+T/hr2BeuE2gyvDrquv6o=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=RhaGFe3b63waVWBP7Snd1z3sMrdALWRgbSBt4CJx3rE1HEJCiIj6hrLg7aMELFpD7 by7f39A+r6MRVe2/u4DCyGcGdCVuREUmDAHY1/EhAeMGjHB9Rm6gsGRkVj4xlHQbrR ljPZB6HAHz+/jbOiL575lf4rfU1agjLs/G7B6sDTuFVLD8rdiyA+YejeTLlBOCRUc2 mQwBj3hTmvhsROvciKqMAe7rLbtGQZK+VghPee0UiFzcXTN+BZGXSnCmDr3BfvzTO4 dfxysYQXnZyitewKkZ4luBi49tq3NKrb3cO5RTpoMY3PcLToa85S9V/T3ww+t77zcL OcXyjJtzqu4Ig== Date: Tue, 26 Oct 2021 11:38:44 -0500 From: Bjorn Helgaas To: Richard Zhu Cc: "l.stach@pengutronix.de" , "bhelgaas@google.com" , "lorenzo.pieralisi@arm.com" , "linux-pci@vger.kernel.org" , dl-linux-imx , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "kernel@pengutronix.de" Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up Message-ID: <20211026163844.GA145569@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote: > > > > > > > > > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{ > > > > > - clk_disable_unprepare(imx6_pcie->pcie); > > > > > - clk_disable_unprepare(imx6_pcie->pcie_phy); > > > > > - clk_disable_unprepare(imx6_pcie->pcie_bus); > > > > > - > > > > > - switch (imx6_pcie->drvdata->variant) { > > > > > - case IMX6SX: > > > > > - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); > > > > > - break; > > > > > - case IMX7D: > > > > > - regmap_update_bits(imx6_pcie->iomuxc_gpr, > > IOMUXC_GPR12, > > > > > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > > > > > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > > > > > - break; > > > > > - case IMX8MQ: > > > > > - clk_disable_unprepare(imx6_pcie->pcie_aux); > > > > > - break; > > > > > - default: > > > > > - break; > > > > > > While you're at it, this "default: break;" thing is pointless. > > > Normally it's better to just *move* something without changing it at > > > all, but this is such a simple thing I think you could drop this at > > > the same time as the move. > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. > [Richard Zhu] I figure out that the default:break is required by > IMX6Q/IMX6QP. So I just don't drop them in v3 patch-set. That makes no sense. The code is: +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) +{ + clk_disable_unprepare(imx6_pcie->pcie); + clk_disable_unprepare(imx6_pcie->pcie_phy); + clk_disable_unprepare(imx6_pcie->pcie_bus); + + switch (imx6_pcie->drvdata->variant) { + case IMX6SX: + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); + break; + case IMX7D: + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); + break; + case IMX8MQ: + clk_disable_unprepare(imx6_pcie->pcie_aux); + break; + default: + break; + } +} Why do you think it makes a difference to remove the "default: break;"? There is no executable code after it. I don't see how IMX6Q/IMX6QP could depend on the default case. BTW, I feel like a broken record, but your v3 posting still has inconsistent subject line capitalization: PCI: imx6: move the clock disable function to a proper place PCI: dwc: add a new callback host exit function into host ops It would be nice if they were consistent and contained more specific information, e.g., PCI: imx6: Move imx6_pcie_clk_disable() earlier PCI: dwc: Add dw_pcie_host_ops.host_exit() callback Bjorn 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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1553FC433F5 for ; Tue, 26 Oct 2021 16:40:05 +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 D767560F02 for ; Tue, 26 Oct 2021 16:40:04 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D767560F02 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org 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:In-Reply-To:MIME-Version:Message-ID: Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:References: List-Owner; bh=Dd1DCaHEVPqZ9+Xzp3Clvr0z2oITibNZRW/V1Gu6rmQ=; b=f7koWutc5qnSff z146AeF0J3VNvfQUCJGa1CLiVY9jW4QszHrSH6Cn2ZXIudK0y86eN0ggfMKkqFdAtIBDJuRTU630s l6U3zTN6AgKn3Y9hNJQhHS/a8Rdo1flyk7kKOyfgus7VeqgnZdUlUf7ffenLWmYOriu3W3By1zi1F 2dK+BycL4LAWm6DXoNDlBomPFuM637o/eoJTc/dk0UcVDunMjqRJEhYUSCBaXd6r6wG93C2m7Zsm+ KLzAhbAmrg+aMfbGZQcBIq++eFRP0ThongRZX0jSM/p+lha2v0KD6FBEPWujTJqE9UG/wZxw74ESU ra270JYH5+BNV3/6elHw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfPTO-002VVW-4w; Tue, 26 Oct 2021 16:38:50 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfPTK-002VUO-RX for linux-arm-kernel@lists.infradead.org; Tue, 26 Oct 2021 16:38:48 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8E62260EC0; Tue, 26 Oct 2021 16:38:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1635266325; bh=M6fXJMrTGa8IExJqf4YZcO+T/hr2BeuE2gyvDrquv6o=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=RhaGFe3b63waVWBP7Snd1z3sMrdALWRgbSBt4CJx3rE1HEJCiIj6hrLg7aMELFpD7 by7f39A+r6MRVe2/u4DCyGcGdCVuREUmDAHY1/EhAeMGjHB9Rm6gsGRkVj4xlHQbrR ljPZB6HAHz+/jbOiL575lf4rfU1agjLs/G7B6sDTuFVLD8rdiyA+YejeTLlBOCRUc2 mQwBj3hTmvhsROvciKqMAe7rLbtGQZK+VghPee0UiFzcXTN+BZGXSnCmDr3BfvzTO4 dfxysYQXnZyitewKkZ4luBi49tq3NKrb3cO5RTpoMY3PcLToa85S9V/T3ww+t77zcL OcXyjJtzqu4Ig== Date: Tue, 26 Oct 2021 11:38:44 -0500 From: Bjorn Helgaas To: Richard Zhu Cc: "l.stach@pengutronix.de" , "bhelgaas@google.com" , "lorenzo.pieralisi@arm.com" , "linux-pci@vger.kernel.org" , dl-linux-imx , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , "kernel@pengutronix.de" Subject: Re: [RESEND v2 4/5] PCI: imx6: Fix the clock reference handling unbalance when link never came up Message-ID: <20211026163844.GA145569@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211026_093846_982051_CED6D657 X-CRM114-Status: GOOD ( 16.59 ) 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 On Fri, Oct 22, 2021 at 08:02:17AM +0000, Richard Zhu wrote: > > > > > > > > > -static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) -{ > > > > > - clk_disable_unprepare(imx6_pcie->pcie); > > > > > - clk_disable_unprepare(imx6_pcie->pcie_phy); > > > > > - clk_disable_unprepare(imx6_pcie->pcie_bus); > > > > > - > > > > > - switch (imx6_pcie->drvdata->variant) { > > > > > - case IMX6SX: > > > > > - clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); > > > > > - break; > > > > > - case IMX7D: > > > > > - regmap_update_bits(imx6_pcie->iomuxc_gpr, > > IOMUXC_GPR12, > > > > > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > > > > > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > > > > > - break; > > > > > - case IMX8MQ: > > > > > - clk_disable_unprepare(imx6_pcie->pcie_aux); > > > > > - break; > > > > > - default: > > > > > - break; > > > > > > While you're at it, this "default: break;" thing is pointless. > > > Normally it's better to just *move* something without changing it at > > > all, but this is such a simple thing I think you could drop this at > > > the same time as the move. > > > > > [Richard Zhu] Okay, got that. Would remove the "default:break" later. Thanks. > [Richard Zhu] I figure out that the default:break is required by > IMX6Q/IMX6QP. So I just don't drop them in v3 patch-set. That makes no sense. The code is: +static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) +{ + clk_disable_unprepare(imx6_pcie->pcie); + clk_disable_unprepare(imx6_pcie->pcie_phy); + clk_disable_unprepare(imx6_pcie->pcie_bus); + + switch (imx6_pcie->drvdata->variant) { + case IMX6SX: + clk_disable_unprepare(imx6_pcie->pcie_inbound_axi); + break; + case IMX7D: + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); + break; + case IMX8MQ: + clk_disable_unprepare(imx6_pcie->pcie_aux); + break; + default: + break; + } +} Why do you think it makes a difference to remove the "default: break;"? There is no executable code after it. I don't see how IMX6Q/IMX6QP could depend on the default case. BTW, I feel like a broken record, but your v3 posting still has inconsistent subject line capitalization: PCI: imx6: move the clock disable function to a proper place PCI: dwc: add a new callback host exit function into host ops It would be nice if they were consistent and contained more specific information, e.g., PCI: imx6: Move imx6_pcie_clk_disable() earlier PCI: dwc: Add dw_pcie_host_ops.host_exit() callback Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel