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 <manivannan.sadhasivam@xxxxxxxxxx> > Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx> > --- > > 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@xxxxxxxxxxxxxxxxxxx > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel