| Message ID | 1584105134-13583-7-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add interconnect support to QSPI and QUP drivers | expand |
On Fri, Mar 13, 2020 at 06:42:12PM +0530, Akash Asthana wrote: > + se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz); > + se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz); As I commented on the previous version to no reply there seem to be a lot of cases where the peak bandwidth is just set to double the normal bandwidth without obvious analysis. Should this default be centralized?
Hi Akash, On Fri, Mar 13, 2020 at 06:42:12PM +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> > --- > - 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 > > drivers/spi/spi-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 73 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c > index c397242..09c4709 100644 > --- a/drivers/spi/spi-geni-qcom.c > +++ b/drivers/spi/spi-geni-qcom.c > @@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz, > return ret; > } > > +static int geni_spi_icc_get(struct geni_se *se) > +{ > + se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core"); > + if (IS_ERR(se->icc_path_geni_to_core)) > + return PTR_ERR(se->icc_path_geni_to_core); > + > + se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config"); > + if (IS_ERR(se->icc_path_cpu_to_geni)) > + return PTR_ERR(se->icc_path_cpu_to_geni); > + > + return 0; > +} As per my comments on (https://patchwork.kernel.org/patch/11436895/#23222713), the above function could be replaced by calling a 'geni_icc_get()' (or so, to be created) provided by the geni SE driver. > + > static void handle_fifo_timeout(struct spi_master *spi, > struct spi_message *msg) > { > @@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv, > return ret; > } > > + /* > + * Set BW quota for CPU as driver supports FIFO mode only. > + * Assume peak bw as twice of avg bw. > + */ > + se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz); > + se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz); > + ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu, > + se->peak_bw_cpu); > + if (ret) { > + dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n", > + __func__); > + 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 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev) > spin_lock_init(&mas->lock); > pm_runtime_enable(dev); > > + ret = geni_spi_icc_get(&mas->se); > + if (ret) > + goto spi_geni_probe_runtime_disable; > + /* Set the bus quota to a reasonable value for register access */ > + mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ); > + mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ); > + mas->se.avg_bw_cpu = Bps_to_icc(1000); > + mas->se.peak_bw_cpu = Bps_to_icc(1000); > + > ret = spi_geni_init(mas); > if (ret) > goto spi_geni_probe_runtime_disable; > @@ -616,14 +652,50 @@ 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); > + ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0); > + if (ret) { > + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n", > + __func__); > + return ret; > + } > + > + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0); > + if (ret) { > + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n", > + __func__); > + return ret; > + } the ICC stuff above would become: ret = geni_icc_vote_off(&mas->se); if (ret) return ret; with the consolidated code in geni SE. > + > + return 0; > } > > 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 = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core, > + mas->se.peak_bw_core); > + if (ret) { > + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n", > + __func__); > + return ret; > + } > + > + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu, > + mas->se.peak_bw_cpu); > + if (ret) { > + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n", > + __func__); > + return ret; > + } and this: ret = geni_icc_vote_on(&mas->se); if (ret) return ret; > return geni_se_resources_on(&mas->se); possibly you could even do the ICC voting from geni_se_resources_on/off() it seems the two are always done together for UART, I2C and SPI.
On 3/13/2020 6:46 PM, Mark Brown wrote: > On Fri, Mar 13, 2020 at 06:42:12PM +0530, Akash Asthana wrote: > >> + se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz); >> + se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz); > As I commented on the previous version to no reply there seem to be a > lot of cases where the peak bandwidth is just set to double the normal > bandwidth without obvious analysis. Should this default be centralized? Hi Mark, I misunderstood previous comment on V1 patch@https://patchwork.kernel.org/patch/11386479/ as a suggestion to mention comment for peak BW choice if nothing mentioned explicitly (in this case I mentioned assume peak BW twice as average). I understand in any case I should have ack'ed the comment atleast by replying "ok". I am sorry about it. We are taking care of actual throughput requirement in avg_bw vote and the intention of putting peak as twice of avg is to ensure that if high speed peripherals(ex:USB) removes their votes, we shouldn't see any latency issue because of other ICC client who don't vote for their BW requirement or *actual* BW requirement. Factor of 2 is chosen randomly. Please correct/improve me if this is not okay. If this is okay, I will centralize this design for SPI QUP, I2C and UART driver. Regards, Akash
Hi Matthias, On 3/14/2020 6:11 AM, Matthias Kaehlcke wrote: > Hi Akash, > > On Fri, Mar 13, 2020 at 06:42:12PM +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> >> --- >> - 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 >> >> drivers/spi/spi-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++- >> 1 file changed, 73 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c >> index c397242..09c4709 100644 >> --- a/drivers/spi/spi-geni-qcom.c >> +++ b/drivers/spi/spi-geni-qcom.c >> @@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz, >> return ret; >> } >> >> +static int geni_spi_icc_get(struct geni_se *se) >> +{ >> + se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core"); >> + if (IS_ERR(se->icc_path_geni_to_core)) >> + return PTR_ERR(se->icc_path_geni_to_core); >> + >> + se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config"); >> + if (IS_ERR(se->icc_path_cpu_to_geni)) >> + return PTR_ERR(se->icc_path_cpu_to_geni); >> + >> + return 0; >> +} > As per my comments on (https://patchwork.kernel.org/patch/11436895/#23222713), > the above function could be replaced by calling a 'geni_icc_get()' (or so, to > be created) provided by the geni SE driver. ok > >> + >> static void handle_fifo_timeout(struct spi_master *spi, >> struct spi_message *msg) >> { >> @@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv, >> return ret; >> } >> >> + /* >> + * Set BW quota for CPU as driver supports FIFO mode only. >> + * Assume peak bw as twice of avg bw. >> + */ >> + se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz); >> + se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz); >> + ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu, >> + se->peak_bw_cpu); >> + if (ret) { >> + dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n", >> + __func__); >> + 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 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev) >> spin_lock_init(&mas->lock); >> pm_runtime_enable(dev); >> >> + ret = geni_spi_icc_get(&mas->se); >> + if (ret) >> + goto spi_geni_probe_runtime_disable; >> + /* Set the bus quota to a reasonable value for register access */ >> + mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ); >> + mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ); >> + mas->se.avg_bw_cpu = Bps_to_icc(1000); >> + mas->se.peak_bw_cpu = Bps_to_icc(1000); >> + >> ret = spi_geni_init(mas); >> if (ret) >> goto spi_geni_probe_runtime_disable; >> @@ -616,14 +652,50 @@ 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); >> + ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0); >> + if (ret) { >> + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n", >> + __func__); >> + return ret; >> + } >> + >> + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0); >> + if (ret) { >> + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n", >> + __func__); >> + return ret; >> + } > the ICC stuff above would become: > > ret = geni_icc_vote_off(&mas->se); > if (ret) > return ret; > > with the consolidated code in geni SE. ok > >> + >> + return 0; >> } >> >> 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 = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core, >> + mas->se.peak_bw_core); >> + if (ret) { >> + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n", >> + __func__); >> + return ret; >> + } >> + >> + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu, >> + mas->se.peak_bw_cpu); >> + if (ret) { >> + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n", >> + __func__); >> + return ret; >> + } > and this: > > ret = geni_icc_vote_on(&mas->se); > if (ret) > return ret; ok >> return geni_se_resources_on(&mas->se); > possibly you could even do the ICC voting from geni_se_resources_on/off() > it seems the two are always done together for UART, I2C and SPI. I think we should expose geni_icc_vote_on/off API seperately and not merge to resources_on/off. Because if we merge then it will appear that we are just doing geni_icc_get() from individual SE driver probe not using any of ICC apis. It looks somewhat asymmetry. Thanks for reviewing, Regards, Akash
On Tue, Mar 17, 2020 at 03:05:21PM +0530, Akash Asthana wrote: > We are taking care of actual throughput requirement in avg_bw vote and the > intention of putting peak as twice of avg is to ensure that if high speed > peripherals(ex:USB) removes their votes, we shouldn't see any latency issue > because of other ICC client who don't vote for their BW requirement or > *actual* BW requirement. Factor of 2 is chosen randomly. Please > correct/improve me if this is not okay. > If this is okay, I will centralize this design for SPI QUP, I2C and UART > driver. That seems reasonable to me, it was just the fact that every driver seemed to be doing the same thing that I was noticing - what was being done seemed OK.
Hi Mark, On 3/17/2020 6:36 PM, Mark Brown wrote: > On Tue, Mar 17, 2020 at 03:05:21PM +0530, Akash Asthana wrote: > >> We are taking care of actual throughput requirement in avg_bw vote and the >> intention of putting peak as twice of avg is to ensure that if high speed >> peripherals(ex:USB) removes their votes, we shouldn't see any latency issue >> because of other ICC client who don't vote for their BW requirement or >> *actual* BW requirement. Factor of 2 is chosen randomly. Please >> correct/improve me if this is not okay. >> If this is okay, I will centralize this design for SPI QUP, I2C and UART >> driver. > That seems reasonable to me, it was just the fact that every driver > seemed to be doing the same thing that I was noticing - what was being > done seemed OK. Okay, thanks for confirming I will keep as is. Regards, Akash
diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c index c397242..09c4709 100644 --- a/drivers/spi/spi-geni-qcom.c +++ b/drivers/spi/spi-geni-qcom.c @@ -118,6 +118,19 @@ static int get_spi_clk_cfg(unsigned int speed_hz, return ret; } +static int geni_spi_icc_get(struct geni_se *se) +{ + se->icc_path_geni_to_core = devm_of_icc_get(se->dev, "qup-core"); + if (IS_ERR(se->icc_path_geni_to_core)) + return PTR_ERR(se->icc_path_geni_to_core); + + se->icc_path_cpu_to_geni = devm_of_icc_get(se->dev, "qup-config"); + if (IS_ERR(se->icc_path_cpu_to_geni)) + return PTR_ERR(se->icc_path_cpu_to_geni); + + return 0; +} + static void handle_fifo_timeout(struct spi_master *spi, struct spi_message *msg) { @@ -234,6 +247,20 @@ static int setup_fifo_params(struct spi_device *spi_slv, return ret; } + /* + * Set BW quota for CPU as driver supports FIFO mode only. + * Assume peak bw as twice of avg bw. + */ + se->avg_bw_cpu = Bps_to_icc(mas->cur_speed_hz); + se->peak_bw_cpu = Bps_to_icc(2 * mas->cur_speed_hz); + ret = icc_set_bw(se->icc_path_cpu_to_geni, se->avg_bw_cpu, + se->peak_bw_cpu); + if (ret) { + dev_err(mas->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + 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 +605,15 @@ static int spi_geni_probe(struct platform_device *pdev) spin_lock_init(&mas->lock); pm_runtime_enable(dev); + ret = geni_spi_icc_get(&mas->se); + if (ret) + goto spi_geni_probe_runtime_disable; + /* Set the bus quota to a reasonable value for register access */ + mas->se.avg_bw_core = Bps_to_icc(CORE_2X_50_MHZ); + mas->se.peak_bw_core = Bps_to_icc(CORE_2X_100_MHZ); + mas->se.avg_bw_cpu = Bps_to_icc(1000); + mas->se.peak_bw_cpu = Bps_to_icc(1000); + ret = spi_geni_init(mas); if (ret) goto spi_geni_probe_runtime_disable; @@ -616,14 +652,50 @@ 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); + ret = icc_set_bw(mas->se.icc_path_geni_to_core, 0, 0); + if (ret) { + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for core\n", + __func__); + return ret; + } + + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, 0, 0); + if (ret) { + dev_err_ratelimited(mas->dev, "%s: ICC BW remove failed for cpu\n", + __func__); + return ret; + } + + return 0; } 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 = icc_set_bw(mas->se.icc_path_geni_to_core, mas->se.avg_bw_core, + mas->se.peak_bw_core); + if (ret) { + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for core\n", + __func__); + return ret; + } + + ret = icc_set_bw(mas->se.icc_path_cpu_to_geni, mas->se.avg_bw_cpu, + mas->se.peak_bw_cpu); + if (ret) { + dev_err_ratelimited(mas->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + return ret; + } return geni_se_resources_on(&mas->se); }
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> --- - 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 drivers/spi/spi-geni-qcom.c | 74 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 1 deletion(-)