| Message ID | 1592222564-13556-7-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hi Mark, do you plan to land this in your tree? I know you hate contentless pings, but since you acked this patch and usually don't seem to do that when patches go through your tree I want to make sure we aren't in a situation where everybody thinks that the patch will go through someone else's tree. Thanks Matthias On Mon, Jun 15, 2020 at 05:32:44PM +0530, Rajendra Nayak wrote: > QSPI needs to vote on a performance state of a power domain depending on > the clock rate. Add support for it by specifying the perf state/clock rate > as an OPP table in device tree. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > Acked-by: Mark Brown <broonie@kernel.org> > Cc: Alok Chauhan <alokc@codeaurora.org> > Cc: Akash Asthana <akashast@codeaurora.org> > Cc: linux-spi@vger.kernel.org > --- > No functional change in v6, rebased over 5.8-rc1 > > drivers/spi/spi-qcom-qspi.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > index 3c4f83b..ef51982 100644 > --- a/drivers/spi/spi-qcom-qspi.c > +++ b/drivers/spi/spi-qcom-qspi.c > @@ -8,6 +8,7 @@ > #include <linux/of.h> > #include <linux/of_platform.h> > #include <linux/pm_runtime.h> > +#include <linux/pm_opp.h> > #include <linux/spi/spi.h> > #include <linux/spi/spi-mem.h> > > @@ -139,6 +140,8 @@ struct qcom_qspi { > struct device *dev; > struct clk_bulk_data *clks; > struct qspi_xfer xfer; > + struct opp_table *opp_table; > + bool has_opp_table; > /* Lock to protect xfer and IRQ accessed registers */ > spinlock_t lock; > }; > @@ -235,7 +238,7 @@ static int qcom_qspi_transfer_one(struct spi_master *master, > speed_hz = xfer->speed_hz; > > /* In regular operation (SBL_EN=1) core must be 4x transfer clock */ > - ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4); > + ret = dev_pm_opp_set_rate(ctrl->dev, speed_hz * 4); > if (ret) { > dev_err(ctrl->dev, "Failed to set core clk %d\n", ret); > return ret; > @@ -481,6 +484,20 @@ static int qcom_qspi_probe(struct platform_device *pdev) > master->handle_err = qcom_qspi_handle_err; > master->auto_runtime_pm = true; > > + ctrl->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); > + if (IS_ERR(ctrl->opp_table)) { > + ret = PTR_ERR(ctrl->opp_table); > + goto exit_probe_master_put; > + } > + /* OPP table is optional */ > + ret = dev_pm_opp_of_add_table(&pdev->dev); > + if (!ret) { > + ctrl->has_opp_table = true; > + } else if (ret != -ENODEV) { > + dev_err(&pdev->dev, "invalid OPP table in device tree\n"); > + goto exit_probe_master_put; > + } > + > pm_runtime_enable(dev); > > ret = spi_register_master(master); > @@ -488,6 +505,9 @@ static int qcom_qspi_probe(struct platform_device *pdev) > return 0; > > pm_runtime_disable(dev); > + if (ctrl->has_opp_table) > + dev_pm_opp_of_remove_table(&pdev->dev); > + dev_pm_opp_put_clkname(ctrl->opp_table); > > exit_probe_master_put: > spi_master_put(master); > @@ -498,11 +518,15 @@ static int qcom_qspi_probe(struct platform_device *pdev) > static int qcom_qspi_remove(struct platform_device *pdev) > { > struct spi_master *master = platform_get_drvdata(pdev); > + struct qcom_qspi *ctrl = spi_master_get_devdata(master); > > /* Unregister _before_ disabling pm_runtime() so we stop transfers */ > spi_unregister_master(master); > > pm_runtime_disable(&pdev->dev); > + if (ctrl->has_opp_table) > + dev_pm_opp_of_remove_table(&pdev->dev); > + dev_pm_opp_put_clkname(ctrl->opp_table); > > return 0; > } > @@ -512,6 +536,8 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev) > struct spi_master *master = dev_get_drvdata(dev); > struct qcom_qspi *ctrl = spi_master_get_devdata(master); > > + /* Drop the performance state vote */ > + dev_pm_opp_set_rate(dev, 0); > clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks); > > return 0; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
On Wed, Jun 24, 2020 at 10:09:33AM -0700, Matthias Kaehlcke wrote: > Hi Mark, > > do you plan to land this in your tree? > > I know you hate contentless pings, but since you acked this patch and > usually don't seem to do that when patches go through your tree I want > to make sure we aren't in a situation where everybody thinks that the > patch will go through someone else's tree. Aren't there dependencies on earlier patches in the series? In general if someone acks something for their tree that means they don't expect to apply it themselves. Please don't top post, reply in line with needed context. This allows readers to readily follow the flow of conversation and understand what you are talking about and also helps ensure that everything in the discussion is being addressed.
On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:09:33AM -0700, Matthias Kaehlcke wrote: > > Hi Mark, > > > > do you plan to land this in your tree? > > > > I know you hate contentless pings, but since you acked this patch and > > usually don't seem to do that when patches go through your tree I want > > to make sure we aren't in a situation where everybody thinks that the > > patch will go through someone else's tree. > > Aren't there dependencies on earlier patches in the series? Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state", however that's not true for this patch. I wonder if it would have been better to split this series into individual patches/mini-series, to avoid this kind of confusion. > In general if someone acks something for their tree that means they don't > expect to apply it themselves. Yes, that was my understanding and prompted me to clarify this with you. The patch could go through the QCOM tree, but to my knowledge there is no reason for it. Btw, the patch "[V8,7/8] spi: spi-qcom-qspi: Add interconnect support" (https://patchwork.kernel.org/patch/11620285/) is in a similar situation. Another patch of the series for the 'spi-geni-qcom' driver has to go through the QCOM change due to changes in geni, but the QSPI driver doesn't use geni and could therefore go through your tree. Ultimately I don't really care too much through which tree the patches land as long as you and Bjorn agree on it :)
On Wed, Jun 24, 2020 at 10:39:48AM -0700, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote: > > Aren't there dependencies on earlier patches in the series? > Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set > clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made > by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state", > however that's not true for this patch. Wait, so *some* of the series should go together but not other bits? But you want them split up for some reason? > I wonder if it would have been better to split this series into individual > patches/mini-series, to avoid this kind of confusion. Yes, if there's no dependencies then bundling things up into a series just causes confusion.
On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:39:48AM -0700, Matthias Kaehlcke wrote: > > On Wed, Jun 24, 2020 at 06:15:37PM +0100, Mark Brown wrote: > > > > Aren't there dependencies on earlier patches in the series? > > > Not to my knowledge. Patch "[2/6] spi: spi-geni-qcom: Use OPP API to set > > clk/perf state" depends on a change in 'include/linux/qcom-geni-se.h' made > > by "1/6] tty: serial: qcom_geni_serial: Use OPP API to set clk/perf state", > > however that's not true for this patch. > > Wait, so *some* of the series should go together but not other bits? > But you want them split up for some reason? Yes, this will almost certainly be the case, even if not for this patch. I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). It seems very unlikely to me that the DRM patches will go through the QCOM tree. The venus patch also doesn't have any dependencies and is more likely to cause conflicts if it lands through QCOM instead of it's maintainer tree. For the QSPI patch you could argue to just take it through QCOM since the SPI patch of this series goes through this tree, up to you, I just want to make sure everybody is on the same page. > > I wonder if it would have been better to split this series into individual > > patches/mini-series, to avoid this kind of confusion. > > Yes, if there's no dependencies then bundling things up into a series > just causes confusion.
On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > > Wait, so *some* of the series should go together but not other bits? > > But you want them split up for some reason? > Yes, this will almost certainly be the case, even if not for this patch. > I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). I'm not really reading any of this stuff for the series as a whole, as far as I could tell I'd reviewed all my bits and was hoping whatever random platform stuff needs sorting out was going to be sorted out so I stopped getting copied on revisions :( > For the QSPI patch you could argue to just take it through QCOM since the SPI > patch of this series goes through this tree, up to you, I just want to make > sure everybody is on the same page. If there are some part of this that don't have a connection with the rest of the series and should be applied separately please split them out and send them separately so it's clear what's going on.
On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: > On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: > > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > > > > Wait, so *some* of the series should go together but not other bits? > > > But you want them split up for some reason? > > > Yes, this will almost certainly be the case, even if not for this patch. > > I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). > > I'm not really reading any of this stuff for the series as a whole, as > far as I could tell I'd reviewed all my bits and was hoping whatever > random platform stuff needs sorting out was going to be sorted out so I > stopped getting copied on revisions :( Sorry this caused you extra work, I only fully realized this when the series was basically ready to land :( Avoiding unnecessary revision spam is another good reason to not combine technically unrelated patches in a single series. If I notice similar series in the future I'll try to bring it up early. > > For the QSPI patch you could argue to just take it through QCOM since the SPI > > patch of this series goes through this tree, up to you, I just want to make > > sure everybody is on the same page. > > If there are some part of this that don't have a connection with the > rest of the series and should be applied separately please split them > out and send them separately so it's clear what's going on. Rajendra, IIUC you have to re-spin this series anyway, please split it up in self-contained chunks.
On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: > > I'm not really reading any of this stuff for the series as a whole, as > > far as I could tell I'd reviewed all my bits and was hoping whatever > > random platform stuff needs sorting out was going to be sorted out so I > > stopped getting copied on revisions :( > Sorry this caused you extra work, I only fully realized this when the series > was basically ready to land :( It's fine, mostly it's just checking to see that I'd reviewed all the relevant patches which takes moments.
On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: > > On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: > > > On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: > > > > > > Wait, so *some* of the series should go together but not other bits? > > > > But you want them split up for some reason? > > > > > Yes, this will almost certainly be the case, even if not for this patch. > > > I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). > > > > I'm not really reading any of this stuff for the series as a whole, as > > far as I could tell I'd reviewed all my bits and was hoping whatever > > random platform stuff needs sorting out was going to be sorted out so I > > stopped getting copied on revisions :( > > Sorry this caused you extra work, I only fully realized this when the series > was basically ready to land :( > > Avoiding unnecessary revision spam is another good reason to not combine > technically unrelated patches in a single series. > > If I notice similar series in the future I'll try to bring it up early. > > > > For the QSPI patch you could argue to just take it through QCOM since the SPI > > > patch of this series goes through this tree, up to you, I just want to make > > > sure everybody is on the same page. > > > > If there are some part of this that don't have a connection with the > > rest of the series and should be applied separately please split them > > out and send them separately so it's clear what's going on. > > Rajendra, IIUC you have to re-spin this series anyway, please split it > up in self-contained chunks. One more thing: when you do the split it seems it would make sense to include the DT changes that were initially part of this series (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=278691&state=*)
On 6/24/2020 11:42 PM, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: >> On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: >>> On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: >> >>>> Wait, so *some* of the series should go together but not other bits? >>>> But you want them split up for some reason? >> >>> Yes, this will almost certainly be the case, even if not for this patch. >>> I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). >> >> I'm not really reading any of this stuff for the series as a whole, as >> far as I could tell I'd reviewed all my bits and was hoping whatever >> random platform stuff needs sorting out was going to be sorted out so I >> stopped getting copied on revisions :( > > Sorry this caused you extra work, I only fully realized this when the series > was basically ready to land :( > > Avoiding unnecessary revision spam is another good reason to not combine > technically unrelated patches in a single series. > > If I notice similar series in the future I'll try to bring it up early. > >>> For the QSPI patch you could argue to just take it through QCOM since the SPI >>> patch of this series goes through this tree, up to you, I just want to make >>> sure everybody is on the same page. >> >> If there are some part of this that don't have a connection with the >> rest of the series and should be applied separately please split them >> out and send them separately so it's clear what's going on. > > Rajendra, IIUC you have to re-spin this series anyway, please split it > up in self-contained chunks. Thanks, I'll respin these as separate patches, the only reason to club them was because they all added 'similar' support in all these different drivers. Sorry for the delay, I had been out a bit and I just got back.
On 6/25/2020 8:55 PM, Matthias Kaehlcke wrote: > On Wed, Jun 24, 2020 at 11:12:45AM -0700, Matthias Kaehlcke wrote: >> On Wed, Jun 24, 2020 at 07:00:05PM +0100, Mark Brown wrote: >>> On Wed, Jun 24, 2020 at 10:55:36AM -0700, Matthias Kaehlcke wrote: >>>> On Wed, Jun 24, 2020 at 06:44:17PM +0100, Mark Brown wrote: >>> >>>>> Wait, so *some* of the series should go together but not other bits? >>>>> But you want them split up for some reason? >>> >>>> Yes, this will almost certainly be the case, even if not for this patch. >>>> I brought this up earlier (https://patchwork.kernel.org/cover/11604623/#23428709). >>> >>> I'm not really reading any of this stuff for the series as a whole, as >>> far as I could tell I'd reviewed all my bits and was hoping whatever >>> random platform stuff needs sorting out was going to be sorted out so I >>> stopped getting copied on revisions :( >> >> Sorry this caused you extra work, I only fully realized this when the series >> was basically ready to land :( >> >> Avoiding unnecessary revision spam is another good reason to not combine >> technically unrelated patches in a single series. >> >> If I notice similar series in the future I'll try to bring it up early. >> >>>> For the QSPI patch you could argue to just take it through QCOM since the SPI >>>> patch of this series goes through this tree, up to you, I just want to make >>>> sure everybody is on the same page. >>> >>> If there are some part of this that don't have a connection with the >>> rest of the series and should be applied separately please split them >>> out and send them separately so it's clear what's going on. >> >> Rajendra, IIUC you have to re-spin this series anyway, please split it >> up in self-contained chunks. > > One more thing: when you do the split it seems it would make sense to > include the DT changes that were initially part of this series > (https://patchwork.kernel.org/project/linux-arm-msm/list/?series=278691&state=*) Sure, I'll send the ones out for which driver changes are already merged/pulled in, like sdhc, geni-uart and geni-spi. For the rest, I will include them with the driver changes.
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c index 3c4f83b..ef51982 100644 --- a/drivers/spi/spi-qcom-qspi.c +++ b/drivers/spi/spi-qcom-qspi.c @@ -8,6 +8,7 @@ #include <linux/of.h> #include <linux/of_platform.h> #include <linux/pm_runtime.h> +#include <linux/pm_opp.h> #include <linux/spi/spi.h> #include <linux/spi/spi-mem.h> @@ -139,6 +140,8 @@ struct qcom_qspi { struct device *dev; struct clk_bulk_data *clks; struct qspi_xfer xfer; + struct opp_table *opp_table; + bool has_opp_table; /* Lock to protect xfer and IRQ accessed registers */ spinlock_t lock; }; @@ -235,7 +238,7 @@ static int qcom_qspi_transfer_one(struct spi_master *master, speed_hz = xfer->speed_hz; /* In regular operation (SBL_EN=1) core must be 4x transfer clock */ - ret = clk_set_rate(ctrl->clks[QSPI_CLK_CORE].clk, speed_hz * 4); + ret = dev_pm_opp_set_rate(ctrl->dev, speed_hz * 4); if (ret) { dev_err(ctrl->dev, "Failed to set core clk %d\n", ret); return ret; @@ -481,6 +484,20 @@ static int qcom_qspi_probe(struct platform_device *pdev) master->handle_err = qcom_qspi_handle_err; master->auto_runtime_pm = true; + ctrl->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core"); + if (IS_ERR(ctrl->opp_table)) { + ret = PTR_ERR(ctrl->opp_table); + goto exit_probe_master_put; + } + /* OPP table is optional */ + ret = dev_pm_opp_of_add_table(&pdev->dev); + if (!ret) { + ctrl->has_opp_table = true; + } else if (ret != -ENODEV) { + dev_err(&pdev->dev, "invalid OPP table in device tree\n"); + goto exit_probe_master_put; + } + pm_runtime_enable(dev); ret = spi_register_master(master); @@ -488,6 +505,9 @@ static int qcom_qspi_probe(struct platform_device *pdev) return 0; pm_runtime_disable(dev); + if (ctrl->has_opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(ctrl->opp_table); exit_probe_master_put: spi_master_put(master); @@ -498,11 +518,15 @@ static int qcom_qspi_probe(struct platform_device *pdev) static int qcom_qspi_remove(struct platform_device *pdev) { struct spi_master *master = platform_get_drvdata(pdev); + struct qcom_qspi *ctrl = spi_master_get_devdata(master); /* Unregister _before_ disabling pm_runtime() so we stop transfers */ spi_unregister_master(master); pm_runtime_disable(&pdev->dev); + if (ctrl->has_opp_table) + dev_pm_opp_of_remove_table(&pdev->dev); + dev_pm_opp_put_clkname(ctrl->opp_table); return 0; } @@ -512,6 +536,8 @@ static int __maybe_unused qcom_qspi_runtime_suspend(struct device *dev) struct spi_master *master = dev_get_drvdata(dev); struct qcom_qspi *ctrl = spi_master_get_devdata(master); + /* Drop the performance state vote */ + dev_pm_opp_set_rate(dev, 0); clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks); return 0;