| Message ID | 1591682194-32388-5-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
On Tue, Jun 09, 2020 at 11:26:31AM +0530, Akash Asthana wrote: > Get the interconnect paths for SPI based Serial Engine device > and vote according to the current bus speed of the driver. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- I've repeatedly acked this patch but my ack never seems to get carried forward :( > + /* Set the bus quota to a reasonable value for register access */ > + mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ); > + mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW; Why are these asymmetric?
Hi Mark, On 6/9/2020 7:11 PM, Mark Brown wrote: > On Tue, Jun 09, 2020 at 11:26:31AM +0530, Akash Asthana wrote: >> Get the interconnect paths for SPI based Serial Engine device >> and vote according to the current bus speed of the driver. >> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- > I've repeatedly acked this patch but my ack never seems to get carried > forward :( I carry acks from previous patches if nothing is changed in current patch wrt previous version, I did that in V6@http://patchwork.ozlabs.org/project/linux-i2c/patch/1590049764-20912-5-git-send-email-akashast@codeaurora.org/ But since there was change from V6 to V7 so, I didn't carried ack from V6 to V7, because I thought I'll need your approvals on additional changes. V7@http://patchwork.ozlabs.org/project/linux-i2c/patch/1590497690-29035-5-git-send-email-akashast@codeaurora.org/ ================================================ Changes in V7: - As per Matthias's comment removed usage of peak_bw variable because we don't have explicit peak requirement, we were voting peak = avg and this can be tracked using single variable for avg bw. ========================================================== > >> + /* Set the bus quota to a reasonable value for register access */ >> + mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ); >> + mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW; > Why are these asymmetric? These are asymmetric because we want to start by putting minimal vote on CPU_TO_GENI path for register access and later based on per transfer's need we scale it at runtime. However, for GENI_TO_CORE path we are trying to keep fixed vote from probe itself that can support max bus speed, we are not scaling it per transfer's need because FW runs on core clock and core behaves a bit different than other NOCs. We don't have any functional relation which mapping btw actual throughput requirement to core frequency need. In the past we have faced few latency issues because of core slowness (Although it was running much higher than actual throughput requirement). To avoid such scenario we are using tested and recommended value from HW team. Thankyou for review. regards, Akash
On Tue, Jun 09, 2020 at 11:26:31AM +0530, Akash Asthana wrote: > Get the interconnect paths for SPI based Serial Engine device > and vote according to the current bus speed of the driver. Acked-by: Mark Brown <broonie@kernel.org>
Hi, On Mon, Jun 8, 2020 at 10:57 PM Akash Asthana <akashast@codeaurora.org> wrote: > > Get the interconnect paths for SPI based Serial Engine device > and vote according to the current bus speed of the driver. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > Reviewed-by: Matthias Kaehlcke <mka@chromium.org> > --- > Changes in V2: > - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get > - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure > - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting > path handle > - As per Matthias comment, added error handling for icc_set_bw call > > Changes in V3: > - As per Matthias's comment, use helper ICC function from geni-se driver. > > Changes in V4: > - Move peak_bw guess as twice of avg_bw if nothing mentioned explicitly > to ICC core. > > Changes in V5: > - Use icc_enable/disable in power on/off call. > - Save some non-zero avg/peak value to ICC core by calling geni_icc_set_bw > from probe so that when resume/icc_enable is called NOC are running at > some non-zero value. No need to call icc_disable after BW vote because > device will resume and suspend before probe return and will leave ICC in > disabled state. > > Changes in V6: > - No change > > Changes in V7: > - As per Matthias's comment removed usage of peak_bw variable because we don't > have explicit peak requirement, we were voting peak = avg and this can be > tracked using single variable for avg bw. > > drivers/spi/spi-geni-qcom.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index c397242..2ace5c5 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -234,6 +234,12 @@ static int setup_fifo_params(struct spi_device *spi_slv, > return ret; > } > > + /* Set BW quota for CPU as driver supports FIFO mode only. */ > + se->icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(mas->cur_speed_hz); > + ret = geni_icc_set_bw(se); > + if (ret) > + return ret; > + I haven't done a deep review of your patch, but a quick drive-by review since I happened to notice it while looking at this driver. You should probably also update the other path that's adjusting the "mas->cur_speed_hz" variable. Specifically see setup_fifo_xfer(). For bonus points, you could even unify the two paths. Perhaps you could pick <https://crrev.com/c/2259624> and include it in your series (remove the WIP if you do). -Doug
Hi Doug, On 6/23/2020 10:36 AM, Doug Anderson wrote: > Hi, > > On Mon, Jun 8, 2020 at 10:57 PM Akash Asthana <akashast@codeaurora.org> wrote: >> Get the interconnect paths for SPI based Serial Engine device >> and vote according to the current bus speed of the driver. >> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >> Reviewed-by: Matthias Kaehlcke <mka@chromium.org> >> --- >> Changes in V2: >> - As per Bjorn's comment, removed se == NULL check from geni_spi_icc_get >> - As per Bjorn's comment, removed code to set se->icc_path* to NULL in failure >> - As per Bjorn's comment, introduced and using devm_of_icc_get API for getting >> path handle >> - As per Matthias comment, added error handling for icc_set_bw call >> >> Changes in V3: >> - As per Matthias's comment, use helper ICC function from geni-se driver. >> >> Changes in V4: >> - Move peak_bw guess as twice of avg_bw if nothing mentioned explicitly >> to ICC core. >> >> Changes in V5: >> - Use icc_enable/disable in power on/off call. >> - Save some non-zero avg/peak value to ICC core by calling geni_icc_set_bw >> from probe so that when resume/icc_enable is called NOC are running at >> some non-zero value. No need to call icc_disable after BW vote because >> device will resume and suspend before probe return and will leave ICC in >> disabled state. >> >> Changes in V6: >> - No change >> >> Changes in V7: >> - As per Matthias's comment removed usage of peak_bw variable because we don't >> have explicit peak requirement, we were voting peak = avg and this can be >> tracked using single variable for avg bw. >> >> drivers/spi/spi-geni-qcom.c | 29 ++++++++++++++++++++++++++++- >> 1 file changed, 28 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >> index c397242..2ace5c5 100644 >> --- a/drivers/spi/spi-geni-qcom.c >> +++ b/drivers/spi/spi-geni-qcom.c >> @@ -234,6 +234,12 @@ static int setup_fifo_params(struct spi_device *spi_slv, >> return ret; >> } >> >> + /* Set BW quota for CPU as driver supports FIFO mode only. */ >> + se->icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(mas->cur_speed_hz); >> + ret = geni_icc_set_bw(se); >> + if (ret) >> + return ret; >> + > I haven't done a deep review of your patch, but a quick drive-by > review since I happened to notice it while looking at this driver. > You should probably also update the other path that's adjusting the > "mas->cur_speed_hz" variable. Specifically see setup_fifo_xfer(). > > For bonus points, you could even unify the two paths. Perhaps you > could pick <https://crrev.com/c/2259624> and include it in your series > (remove the WIP if you do). Yeah, we can adjust ICC vote per transfer like we are doing for clock. I will include your patch to v8 series. Thanks for review. regards, Akash > > -Doug
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index c397242..2ace5c5 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -234,6 +234,12 @@ static int setup_fifo_params(struct spi_device *spi_slv, return ret; } + /* Set BW quota for CPU as driver supports FIFO mode only. */ + se->icc_paths[CPU_TO_GENI].avg_bw = Bps_to_icc(mas->cur_speed_hz); + ret = geni_icc_set_bw(se); + if (ret) + return ret; + clk_sel = idx & CLK_SEL_MSK; m_clk_cfg = (div << CLK_DIV_SHFT) | SER_CLK_EN; spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word); @@ -578,6 +584,17 @@ static int spi_geni_probe(struct platform_device *pdev) spin_lock_init(&mas->lock); pm_runtime_enable(dev); + ret = geni_icc_get(&mas->se, NULL); + if (ret) + goto spi_geni_probe_runtime_disable; + /* Set the bus quota to a reasonable value for register access */ + mas->se.icc_paths[GENI_TO_CORE].avg_bw = Bps_to_icc(CORE_2X_50_MHZ); + mas->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW; + + ret = geni_icc_set_bw(&mas->se); + if (ret) + goto spi_geni_probe_runtime_disable; + ret = spi_geni_init(mas); if (ret) goto spi_geni_probe_runtime_disable; @@ -616,14 +633,24 @@ static int __maybe_unused spi_geni_runtime_suspend(struct device *dev) { struct spi_master *spi = dev_get_drvdata(dev); struct spi_geni_master *mas = spi_master_get_devdata(spi); + int ret; + + ret = geni_se_resources_off(&mas->se); + if (ret) + return ret; - return geni_se_resources_off(&mas->se); + return geni_icc_disable(&mas->se); } static int __maybe_unused spi_geni_runtime_resume(struct device *dev) { struct spi_master *spi = dev_get_drvdata(dev); struct spi_geni_master *mas = spi_master_get_devdata(spi); + int ret; + + ret = geni_icc_enable(&mas->se); + if (ret) + return ret; return geni_se_resources_on(&mas->se); }