Ensure the *_enable_ref_clk() function is symmetric by addressing missing
disable parts on some platforms.
Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable")
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 413db182ce9f..ab2c97a8c327 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -599,10 +599,9 @@ static int imx_pcie_attach_pd(struct device *dev)
static int imx6sx_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
{
- if (enable)
- regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
-
+ regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
+ enable ? 0 : IMX6SX_GPR12_PCIE_TEST_POWERDOWN);
return 0;
}
@@ -631,19 +630,20 @@ static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
{
int offset = imx_pcie_grp_offset(imx_pcie);
- if (enable) {
- regmap_clear_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE);
- regmap_set_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN);
- }
-
+ regmap_update_bits(imx_pcie->iomuxc_gpr, offset,
+ IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
+ enable ? 0 : IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE);
+ regmap_update_bits(imx_pcie->iomuxc_gpr, offset,
+ IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN,
+ enable ? IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN : 0);
return 0;
}
static int imx7d_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable)
{
- if (!enable)
- regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
- IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
+ regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12,
+ IMX7D_GPR12_PCIE_PHY_REFCLK_SEL,
+ enable ? 0 : IMX7D_GPR12_PCIE_PHY_REFCLK_SEL);
return 0;
}
--
2.37.1
On Tue, Nov 26, 2024 at 03:56:58PM +0800, Richard Zhu wrote: > Ensure the *_enable_ref_clk() function is symmetric by addressing missing > disable parts on some platforms. > > Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable") The patch below looks fine to me, and I guess it's more than just making the code prettier; it also actually *fixes* something, right? It looks like a functional change because imx_pcie_clk_enable() will now enable the IMX7D refclk when it didn't before, and imx_pcie_clk_disable() will disable the IMX6SX and IMX8M* refclk when it didn't before? But I don't think the Fixes: tag is correct. I looked at uses of these symbols: IMX6SX_GPR12_PCIE_TEST_POWERDOWN enabled by imx6_pcie_enable_ref_clk() disabled by imx6_pcie_assert_core_reset() IMX7D_GPR12_PCIE_PHY_REFCLK_SEL enabled by imx6_pcie_init_phy() disabled by imx6_pcie_clk_disable() IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE enabled by imx6_pcie_enable_ref_clk() As far as I can tell, these uses are identical before and after d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable"). > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > index 413db182ce9f..ab2c97a8c327 100644 > --- a/drivers/pci/controller/dwc/pci-imx6.c > +++ b/drivers/pci/controller/dwc/pci-imx6.c > @@ -599,10 +599,9 @@ static int imx_pcie_attach_pd(struct device *dev) > > static int imx6sx_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > { > - if (enable) > - regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > - IMX6SX_GPR12_PCIE_TEST_POWERDOWN); > - > + regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > + IMX6SX_GPR12_PCIE_TEST_POWERDOWN, > + enable ? 0 : IMX6SX_GPR12_PCIE_TEST_POWERDOWN); > return 0; > } > > @@ -631,19 +630,20 @@ static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > { > int offset = imx_pcie_grp_offset(imx_pcie); > > - if (enable) { > - regmap_clear_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE); > - regmap_set_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN); > - } > - > + regmap_update_bits(imx_pcie->iomuxc_gpr, offset, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE, > + enable ? 0 : IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE); > + regmap_update_bits(imx_pcie->iomuxc_gpr, offset, > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN, > + enable ? IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN : 0); > return 0; > } > > static int imx7d_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > { > - if (!enable) > - regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > + regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > + enable ? 0 : IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > return 0; > } > > -- > 2.37.1 >
On Thu, Jan 16, 2025 at 11:01:14AM -0600, Bjorn Helgaas wrote: > On Tue, Nov 26, 2024 at 03:56:58PM +0800, Richard Zhu wrote: > > Ensure the *_enable_ref_clk() function is symmetric by addressing missing > > disable parts on some platforms. > > > > Fixes: d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match enable") > > The patch below looks fine to me, and I guess it's more than just > making the code prettier; it also actually *fixes* something, right? > > It looks like a functional change because imx_pcie_clk_enable() will > now enable the IMX7D refclk when it didn't before, and > imx_pcie_clk_disable() will disable the IMX6SX and IMX8M* refclk when > it didn't before? > > But I don't think the Fixes: tag is correct. I looked at uses of > these symbols: > > IMX6SX_GPR12_PCIE_TEST_POWERDOWN > enabled by imx6_pcie_enable_ref_clk() > disabled by imx6_pcie_assert_core_reset() > > IMX7D_GPR12_PCIE_PHY_REFCLK_SEL > enabled by imx6_pcie_init_phy() > disabled by imx6_pcie_clk_disable() > > IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE > enabled by imx6_pcie_enable_ref_clk() > > As far as I can tell, these uses are identical before and after > d0a75c791f98 ("PCI: imx6: Factor out ref clock disable to match > enable"). You can drop fixes tags Frank > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > --- > > drivers/pci/controller/dwc/pci-imx6.c | 24 ++++++++++++------------ > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c > > index 413db182ce9f..ab2c97a8c327 100644 > > --- a/drivers/pci/controller/dwc/pci-imx6.c > > +++ b/drivers/pci/controller/dwc/pci-imx6.c > > @@ -599,10 +599,9 @@ static int imx_pcie_attach_pd(struct device *dev) > > > > static int imx6sx_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > > { > > - if (enable) > > - regmap_clear_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX6SX_GPR12_PCIE_TEST_POWERDOWN); > > - > > + regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > + IMX6SX_GPR12_PCIE_TEST_POWERDOWN, > > + enable ? 0 : IMX6SX_GPR12_PCIE_TEST_POWERDOWN); > > return 0; > > } > > > > @@ -631,19 +630,20 @@ static int imx8mm_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > > { > > int offset = imx_pcie_grp_offset(imx_pcie); > > > > - if (enable) { > > - regmap_clear_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE); > > - regmap_set_bits(imx_pcie->iomuxc_gpr, offset, IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN); > > - } > > - > > + regmap_update_bits(imx_pcie->iomuxc_gpr, offset, > > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE, > > + enable ? 0 : IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE); > > + regmap_update_bits(imx_pcie->iomuxc_gpr, offset, > > + IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN, > > + enable ? IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN : 0); > > return 0; > > } > > > > static int imx7d_pcie_enable_ref_clk(struct imx_pcie *imx_pcie, bool enable) > > { > > - if (!enable) > > - regmap_set_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > - IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > > + regmap_update_bits(imx_pcie->iomuxc_gpr, IOMUXC_GPR12, > > + IMX7D_GPR12_PCIE_PHY_REFCLK_SEL, > > + enable ? 0 : IMX7D_GPR12_PCIE_PHY_REFCLK_SEL); > > return 0; > > } > > > > -- > > 2.37.1 > >
© 2016 - 2025 Red Hat, Inc.