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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 9E550C4829A for ; Tue, 13 Feb 2024 11:41:38 +0000 (UTC) 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:References: 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: List-Owner; bh=Zz/msT3QTbHggECJ+ZIyWhrpTOerT6/kH4vwH5BTmrk=; b=KfKC+dURyUMmIX qSPFHCFyCxpmitHPDLEP6Uu3SnOnEexieS8k+1xdPna5T4ECv+81vP6eNuhoK32vKukr0W0Y/UV2b nXr3uLtLAFPGqocInhkrxxtj3IRzOfveiJSluGu9dMThn1+3GS8zvH/gotuobiuAddBGz1Fwrv0kJ CIecM27aLUuIvOEhb7xDsnd0oVcxwpPXIi06hwokY0bbErkAX3HbtrD778A68l1tG7+JzhVbDFgBT 5HEhpU+3gcOPb+T0FyA+S3/v/BhA3btDODCLkRYNyO7QQUsJlnP3Bm3KXfF05yKQsC0mAzk4yQBX4 sq4RUTUPVzJlGcaccaew==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZrA9-000000093cq-455p; Tue, 13 Feb 2024 11:41:21 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rZrA7-000000093c3-0Lq9 for linux-arm-kernel@lists.infradead.org; Tue, 13 Feb 2024 11:41:20 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 73F8F61382; Tue, 13 Feb 2024 11:41:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9924EC433C7; Tue, 13 Feb 2024 11:41:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1707824478; bh=vcE4VEnsqCeEFS58nCHqwFF96GXVJtY39V+7YxzpGJQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=D9hFuNLeBmxFMZ2fEls4m360KhrHa7/r5yzhaHxZEIJJ/KOMNQaIspdeJ6qNkAGf7 om3V5xII6BmPVrnUd+Gf2uSJD54jTLiZLemsNeEsijEQG0JueGpvUWEphph8M7aCrb UGKj9B2TRyQFgBNV2pi0J7wsXaUedkxT2VibHAVnD+185w1/MlhQfYVuY3HKi6xJ9+ eX+b3gJxUIo/otji1LhMgRhQhd3HloVXA2gJxSrHdHipV0HZR2kUIwRxu1D0lMlUlR ZM5sdFoNfW2bkFzqBu4rX8JoFSKhrdZyskDt1Sttj04HvEeTme61/Vt4cTqLht2T5U tGTaG/DoDvDBA== Date: Tue, 13 Feb 2024 12:41:10 +0100 From: Lorenzo Pieralisi To: Frank Li Cc: bhelgaas@google.com, conor+dt@kernel.org, devicetree@vger.kernel.org, festevam@gmail.com, helgaas@kernel.org, hongxing.zhu@nxp.com, imx@lists.linux.dev, kernel@pengutronix.de, krzysztof.kozlowski+dt@linaro.org, krzysztof.kozlowski@linaro.org, kw@linux.com, l.stach@pengutronix.de, linux-arm-kernel@lists.infradead.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org, manivannan.sadhasivam@linaro.org, robh@kernel.org, s.hauer@pengutronix.de, shawnguo@kernel.org Subject: Re: [PATCH v10 03/14] PCI: imx6: Simplify reset handling by using by using *_FLAG_HAS_*_RESET Message-ID: References: <20240205173335.1120469-1-Frank.Li@nxp.com> <20240205173335.1120469-4-Frank.Li@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20240205173335.1120469-4-Frank.Li@nxp.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240213_034119_257723_AC126602 X-CRM114-Status: GOOD ( 30.82 ) 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 Mon, Feb 05, 2024 at 12:33:24PM -0500, Frank Li wrote: > Refactors the reset handling logic in the imx6 PCI driver by adding > IMX6_PCIE_FLAG_HAS_*_RESET bitmask define for drvdata::flags. > > The drvdata::flags and a bitmask ensures a cleaner and more scalable > switch-case structure for handling reset. > > Reviewed-by: Manivannan Sadhasivam > Reviewed-by: Philipp Zabel > Signed-off-by: Frank Li > --- > > Notes: > Change from v4 to v5: > - Add Mani's Reviewed-by tag > - Fixed MQ_EP's flags > > Chagne from v3 to v4: > - none > Change from v2 to v3: > - add Philipp's Reviewed-by tag > Change from v1 to v2: > - remove condition check before reset_control_(de)assert() because it is > none ops if a NULL pointer pass down. > - still keep condition check at probe to help identify dts file mismatch > problem. > > Change from v1 to v2: > - remove condition check before reset_control_(de)assert() because it is > none ops if a NULL pointer pass down. > - still keep condition check at probe to help identify dts file mismatch > problem. > > drivers/pci/controller/dwc/pci-imx6.c | 105 ++++++++++---------------- > 1 file changed, 39 insertions(+), 66 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 59f117f855c26..a1653b58051b7 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -61,6 +61,8 @@ enum imx6_pcie_variants { > #define IMX6_PCIE_FLAG_IMX6_SPEED_CHANGE BIT(1) > #define IMX6_PCIE_FLAG_SUPPORTS_SUSPEND BIT(2) > #define IMX6_PCIE_FLAG_HAS_PHYDRV BIT(3) > +#define IMX6_PCIE_FLAG_HAS_APP_RESET BIT(4) > +#define IMX6_PCIE_FLAG_HAS_PHY_RESET BIT(5) > > #define imx6_check_flag(pci, val) (pci->drvdata->flags & val) > > @@ -661,18 +663,10 @@ static void imx6_pcie_clk_disable(struct imx6_pcie *imx6_pcie) > > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > { > + reset_control_assert(imx6_pcie->pciephy_reset); > + reset_control_assert(imx6_pcie->apps_reset); > + > switch (imx6_pcie->drvdata->variant) { > - case IMX7D: > - case IMX8MQ: > - case IMX8MQ_EP: > - reset_control_assert(imx6_pcie->pciephy_reset); > - fallthrough; > - case IMX8MM: > - case IMX8MM_EP: > - case IMX8MP: > - case IMX8MP_EP: > - reset_control_assert(imx6_pcie->apps_reset); > - break; > case IMX6SX: > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX6SX_GPR12_PCIE_TEST_POWERDOWN, > @@ -693,6 +687,8 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie) > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > IMX6Q_GPR1_PCIE_REF_CLK_EN, 0 << 16); > break; > + default: > + break; > } > > /* Some boards don't have PCIe reset GPIO. */ > @@ -706,14 +702,10 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > struct dw_pcie *pci = imx6_pcie->pci; > struct device *dev = pci->dev; > > + reset_control_deassert(imx6_pcie->pciephy_reset); > + > switch (imx6_pcie->drvdata->variant) { > - case IMX8MQ: > - case IMX8MQ_EP: > - reset_control_deassert(imx6_pcie->pciephy_reset); > - break; > case IMX7D: > - reset_control_deassert(imx6_pcie->pciephy_reset); > - > /* Workaround for ERR010728, failure of PCI-e PLL VCO to > * oscillate, especially when cold. This turns off "Duty-cycle > * Corrector" and other mysterious undocumented things. > @@ -745,11 +737,7 @@ static int imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie) > > usleep_range(200, 500); > break; > - case IMX6Q: /* Nothing to do */ > - case IMX8MM: > - case IMX8MM_EP: > - case IMX8MP: > - case IMX8MP_EP: > + default: > break; > } > > @@ -796,16 +784,11 @@ static void imx6_pcie_ltssm_enable(struct device *dev) > IMX6Q_GPR12_PCIE_CTL_2, > IMX6Q_GPR12_PCIE_CTL_2); > break; > - case IMX7D: > - case IMX8MQ: > - case IMX8MQ_EP: > - case IMX8MM: > - case IMX8MM_EP: > - case IMX8MP: > - case IMX8MP_EP: > - reset_control_deassert(imx6_pcie->apps_reset); > + default: > break; > } > + > + reset_control_deassert(imx6_pcie->apps_reset); You rely on the fact that passing NULL is a no-op on platforms where this wasn't called before (valid question for other hunks in this commit), correct ? Just checking, it does not make things much cleaner but I am not opposed to this change. Thanks, Lorenzo > } > > static void imx6_pcie_ltssm_disable(struct device *dev) > @@ -819,16 +802,11 @@ static void imx6_pcie_ltssm_disable(struct device *dev) > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, > IMX6Q_GPR12_PCIE_CTL_2, 0); > break; > - case IMX7D: > - case IMX8MQ: > - case IMX8MQ_EP: > - case IMX8MM: > - case IMX8MM_EP: > - case IMX8MP: > - case IMX8MP_EP: > - reset_control_assert(imx6_pcie->apps_reset); > + default: > break; > } > + > + reset_control_assert(imx6_pcie->apps_reset); > } > > static int imx6_pcie_start_link(struct dw_pcie *pci) > @@ -1287,38 +1265,26 @@ static int imx6_pcie_probe(struct platform_device *pdev) > "failed to get pcie phy\n"); > } > > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_APP_RESET)) { > + imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, "apps"); > + if (IS_ERR(imx6_pcie->apps_reset)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset), > + "failed to get pcie apps reset control\n"); > + } > + > + if (imx6_check_flag(imx6_pcie, IMX6_PCIE_FLAG_HAS_PHY_RESET)) { > + imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev, "pciephy"); > + if (IS_ERR(imx6_pcie->pciephy_reset)) > + return dev_err_probe(dev, PTR_ERR(imx6_pcie->pciephy_reset), > + "Failed to get PCIEPHY reset control\n"); > + } > + > switch (imx6_pcie->drvdata->variant) { > case IMX8MQ: > case IMX8MQ_EP: > case IMX7D: > if (dbi_base->start == IMX8MQ_PCIE2_BASE_ADDR) > imx6_pcie->controller_id = 1; > - > - imx6_pcie->pciephy_reset = devm_reset_control_get_exclusive(dev, > - "pciephy"); > - if (IS_ERR(imx6_pcie->pciephy_reset)) { > - dev_err(dev, "Failed to get PCIEPHY reset control\n"); > - return PTR_ERR(imx6_pcie->pciephy_reset); > - } > - > - imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, > - "apps"); > - if (IS_ERR(imx6_pcie->apps_reset)) { > - dev_err(dev, "Failed to get PCIE APPS reset control\n"); > - return PTR_ERR(imx6_pcie->apps_reset); > - } > - break; > - case IMX8MM: > - case IMX8MM_EP: > - case IMX8MP: > - case IMX8MP_EP: > - imx6_pcie->apps_reset = devm_reset_control_get_exclusive(dev, > - "apps"); > - if (IS_ERR(imx6_pcie->apps_reset)) > - return dev_err_probe(dev, PTR_ERR(imx6_pcie->apps_reset), > - "failed to get pcie apps reset control\n"); > - > - break; > default: > break; > } > @@ -1448,13 +1414,17 @@ static const struct imx6_pcie_drvdata drvdata[] = { > }, > [IMX7D] = { > .variant = IMX7D, > - .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND, > + .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > + IMX6_PCIE_FLAG_HAS_APP_RESET | > + IMX6_PCIE_FLAG_HAS_PHY_RESET, > .gpr = "fsl,imx7d-iomuxc-gpr", > .clk_names = imx6q_clks, > .clks_cnt = ARRAY_SIZE(imx6q_clks), > }, > [IMX8MQ] = { > .variant = IMX8MQ, > + .flags = IMX6_PCIE_FLAG_HAS_APP_RESET | > + IMX6_PCIE_FLAG_HAS_PHY_RESET, > .gpr = "fsl,imx8mq-iomuxc-gpr", > .clk_names = imx8mq_clks, > .clks_cnt = ARRAY_SIZE(imx8mq_clks), > @@ -1471,13 +1441,16 @@ static const struct imx6_pcie_drvdata drvdata[] = { > [IMX8MP] = { > .variant = IMX8MP, > .flags = IMX6_PCIE_FLAG_SUPPORTS_SUSPEND | > - IMX6_PCIE_FLAG_HAS_PHYDRV, > + IMX6_PCIE_FLAG_HAS_PHYDRV | > + IMX6_PCIE_FLAG_HAS_APP_RESET, > .gpr = "fsl,imx8mp-iomuxc-gpr", > .clk_names = imx8mm_clks, > .clks_cnt = ARRAY_SIZE(imx8mm_clks), > }, > [IMX8MQ_EP] = { > .variant = IMX8MQ_EP, > + .flags = IMX6_PCIE_FLAG_HAS_APP_RESET | > + IMX6_PCIE_FLAG_HAS_PHY_RESET, > .mode = DW_PCIE_EP_TYPE, > .gpr = "fsl,imx8mq-iomuxc-gpr", > .clk_names = imx8mq_clks, > -- > 2.34.1 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel