| Message ID | 20210416004652.2975446-2-quanyang.wang@windriver.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: spi-zynqmp-gqspi: fix spi issues | expand |
On Fri, Apr 16, 2021 at 08:46:48AM +0800, quanyang.wang@windriver.com wrote: > Since pm_runtime works now, clks can be enabled/disabled by calling > zynqmp_runtime_suspend/resume. So we don't need to enable these clks > explicitly in zynqmp_qspi_setup_op. Remove them to fix this issue. > Fixes: 1c26372e5aa9 ("spi: spi-zynqmp-gqspi: Update driver to use spi-mem framework") Are you *sure* this fixes is accurate? The patch (and several of the others that flag the same commit) doesn't apply against for-5.12, though at this point there's not really enough time to send another pull request so it doesn't super matter though someone will probably need to help out with stable backports.
Hi Mark, On 4/16/21 8:55 PM, Mark Brown wrote: > On Fri, Apr 16, 2021 at 08:46:48AM +0800, quanyang.wang@windriver.com wrote: > >> Since pm_runtime works now, clks can be enabled/disabled by calling >> zynqmp_runtime_suspend/resume. So we don't need to enable these clks >> explicitly in zynqmp_qspi_setup_op. Remove them to fix this issue. >> Fixes: 1c26372e5aa9 ("spi: spi-zynqmp-gqspi: Update driver to use spi-mem framework") > Are you *sure* this fixes is accurate? The patch (and several of the > others that flag the same commit) doesn't apply against for-5.12, though > at this point there's not really enough time to send another pull request > so it doesn't super matter though someone will probably need to help out > with stable backports. I am sorry. These patches should NOT be with "Fixes" tag since they base on the patches which are not with "Fixes". May I send a V2 patch series which remove these "Fixes" tags? Thanks, Quanyang
On Fri, Apr 16, 2021 at 10:04:30PM +0800, quanyang.wang wrote: > I am sorry. These patches should NOT be with "Fixes" tag since they base on > the patches > which are not with "Fixes". May I send a V2 patch series which remove these > "Fixes" tags? Well, if they're fixing bugs that were present in the named commit then the the tag makes sense, it's just a question of if they are actually for that commit or if they are fixing things for other commits like the runtime PM enablement.
Hi Mark, On 4/16/21 11:12 PM, Mark Brown wrote: > On Fri, Apr 16, 2021 at 10:04:30PM +0800, quanyang.wang wrote: > >> I am sorry. These patches should NOT be with "Fixes" tag since they base on >> the patches >> which are not with "Fixes". May I send a V2 patch series which remove these >> "Fixes" tags? > Well, if they're fixing bugs that were present in the named commit then > the the tag makes sense, it's just a question of if they are actually > for that commit or if they are fixing things for other commits like the > runtime PM enablement. Yes, they are fixing bugs which are introduced by the named commit. But if only picking they to stable without the patch "spi: spi-zynqmp-gqspi: Fix runtime PM imbalance in zynqmp_qspi_probe", this spi driver will not work. I don't know how to handle this condition, so I send a V2 patch series to delete the tags. Thanks, Quanyang
diff --git a/drivers/spi/spi-zynqmp-gqspi.c b/drivers/spi/spi-zynqmp-gqspi.c index 32e53f379e9b..f9056f0a480c 100644 --- a/drivers/spi/spi-zynqmp-gqspi.c +++ b/drivers/spi/spi-zynqmp-gqspi.c @@ -487,24 +487,10 @@ static int zynqmp_qspi_setup_op(struct spi_device *qspi) { struct spi_controller *ctlr = qspi->master; struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr); - struct device *dev = &ctlr->dev; - int ret; if (ctlr->busy) return -EBUSY; - ret = clk_enable(xqspi->refclk); - if (ret) { - dev_err(dev, "Cannot enable device clock.\n"); - return ret; - } - - ret = clk_enable(xqspi->pclk); - if (ret) { - dev_err(dev, "Cannot enable APB clock.\n"); - clk_disable(xqspi->refclk); - return ret; - } zynqmp_gqspi_write(xqspi, GQSPI_EN_OFST, GQSPI_EN_MASK); return 0; @@ -863,26 +849,9 @@ static int __maybe_unused zynqmp_qspi_suspend(struct device *dev) static int __maybe_unused zynqmp_qspi_resume(struct device *dev) { struct spi_controller *ctlr = dev_get_drvdata(dev); - struct zynqmp_qspi *xqspi = spi_controller_get_devdata(ctlr); - int ret = 0; - - ret = clk_enable(xqspi->pclk); - if (ret) { - dev_err(dev, "Cannot enable APB clock.\n"); - return ret; - } - - ret = clk_enable(xqspi->refclk); - if (ret) { - dev_err(dev, "Cannot enable device clock.\n"); - clk_disable(xqspi->pclk); - return ret; - } spi_controller_resume(ctlr); - clk_disable(xqspi->refclk); - clk_disable(xqspi->pclk); return 0; } @@ -898,8 +867,8 @@ static int __maybe_unused zynqmp_runtime_suspend(struct device *dev) { struct zynqmp_qspi *xqspi = (struct zynqmp_qspi *)dev_get_drvdata(dev); - clk_disable(xqspi->refclk); - clk_disable(xqspi->pclk); + clk_disable_unprepare(xqspi->refclk); + clk_disable_unprepare(xqspi->pclk); return 0; } @@ -917,16 +886,16 @@ static int __maybe_unused zynqmp_runtime_resume(struct device *dev) struct zynqmp_qspi *xqspi = (struct zynqmp_qspi *)dev_get_drvdata(dev); int ret; - ret = clk_enable(xqspi->pclk); + ret = clk_prepare_enable(xqspi->pclk); if (ret) { dev_err(dev, "Cannot enable APB clock.\n"); return ret; } - ret = clk_enable(xqspi->refclk); + ret = clk_prepare_enable(xqspi->refclk); if (ret) { dev_err(dev, "Cannot enable device clock.\n"); - clk_disable(xqspi->pclk); + clk_disable_unprepare(xqspi->pclk); return ret; } @@ -1136,13 +1105,11 @@ static int zynqmp_qspi_probe(struct platform_device *pdev) goto remove_master; } - init_completion(&xqspi->data_completion); - xqspi->refclk = devm_clk_get(&pdev->dev, "ref_clk"); if (IS_ERR(xqspi->refclk)) { dev_err(dev, "ref_clk clock not found.\n"); ret = PTR_ERR(xqspi->refclk); - goto clk_dis_pclk; + goto remove_master; } ret = clk_prepare_enable(xqspi->pclk); @@ -1157,6 +1124,8 @@ static int zynqmp_qspi_probe(struct platform_device *pdev) goto clk_dis_pclk; } + init_completion(&xqspi->data_completion); + mutex_init(&xqspi->op_lock); pm_runtime_use_autosuspend(&pdev->dev);