| Message ID | 1585652976-17481-8-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, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote: > + /* > + * Set BW quota for CPU as driver supports FIFO mode only. > + * Assume peak bw as twice of avg bw. > + */ > + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); > + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); I thought you were going to factor this best guess handling of peak bandwidth out into the core?
On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote: > Get the interconnect paths for QSPI device and vote according to the > current bus speed of the driver. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > --- > Changes in V2: > - 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: > - No Change. > > drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c > index 3c4f83b..ad48f43 100644 > --- a/drivers/spi/spi-qcom-qspi.c > +++ b/drivers/spi/spi-qcom-qspi.c > @@ -2,6 +2,7 @@ > // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. > > #include <linux/clk.h> > +#include <linux/interconnect.h> > #include <linux/interrupt.h> > #include <linux/io.h> > #include <linux/module.h> > @@ -139,7 +140,10 @@ struct qcom_qspi { > struct device *dev; > struct clk_bulk_data *clks; > struct qspi_xfer xfer; > - /* Lock to protect xfer and IRQ accessed registers */ > + struct icc_path *icc_path_cpu_to_qspi; > + unsigned int avg_bw_cpu; > + unsigned int peak_bw_cpu; > + /* Lock to protect data accessed by IRQs */ > spinlock_t lock; > }; > > @@ -241,6 +245,20 @@ static int qcom_qspi_transfer_one(struct spi_master *master, > return ret; > } > > + /* > + * Set BW quota for CPU as driver supports FIFO mode only. > + * Assume peak bw as twice of avg bw. > + */ > + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); > + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); > + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, > + ctrl->peak_bw_cpu); > + if (ret) { > + dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu\n", > + __func__); > + return ret; > + } > + > spin_lock_irqsave(&ctrl->lock, flags); > > /* We are half duplex, so either rx or tx will be set */ > @@ -458,6 +476,15 @@ static int qcom_qspi_probe(struct platform_device *pdev) > if (ret) > goto exit_probe_master_put; > > + ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config"); > + if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) { > + ret = PTR_ERR(ctrl->icc_path_cpu_to_qspi); > + goto exit_probe_master_put; > + } > + /* Put BW vote on CPU path for register access */ > + ctrl->avg_bw_cpu = Bps_to_icc(1000); > + ctrl->peak_bw_cpu = Bps_to_icc(1000); > + > ret = platform_get_irq(pdev, 0); > if (ret < 0) > goto exit_probe_master_put; > @@ -511,9 +538,17 @@ 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); > + int ret; > > clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks); > > + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, 0, 0); > + if (ret) { > + dev_err_ratelimited(ctrl->dev, "%s: ICC BW remove failed for cpu\n", > + __func__); > + return ret; > + } > + > return 0; > } > > @@ -521,6 +556,15 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev) > { > struct spi_master *master = dev_get_drvdata(dev); > struct qcom_qspi *ctrl = spi_master_get_devdata(master); > + int ret; > + > + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, > + ctrl->peak_bw_cpu); > + if (ret) { > + dev_err_ratelimited(ctrl->dev, "%s: ICC BW voting failed for cpu\n", > + __func__); > + return ret; > + } > > return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks); > } Looks good to me besides Mark's concern about the bandwith calculation logic. Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Hi Mark, On 3/31/2020 4:53 PM, Mark Brown wrote: > On Tue, Mar 31, 2020 at 04:39:35PM +0530, Akash Asthana wrote: > >> + /* >> + * Set BW quota for CPU as driver supports FIFO mode only. >> + * Assume peak bw as twice of avg bw. >> + */ >> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); > I thought you were going to factor this best guess handling of peak > bandwidth out into the core? I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) but still for QSPI I have to keep this piece of code as is because It is not child of QUP wrapper(it doesn't use common code). I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if nothing is mentioned explicitly" to ICC core because the factor of 2 is chosen randomly by me. Regards, Akash
On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: > On 3/31/2020 4:53 PM, Mark Brown wrote: > > > + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); > > > + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); > > I thought you were going to factor this best guess handling of peak > > bandwidth out into the core? > I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) > but still for QSPI I have to keep this piece of code as is because It is not > child of QUP wrapper(it doesn't use common code). Why not? > I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if > nothing is mentioned explicitly" to ICC core because the factor of 2 is > chosen randomly by me. That's the whole point - if this is just a random number then we may as well at least be consistently random.
Hi Mark, Evan, Georgi, On 4/7/2020 4:25 PM, Mark Brown wrote: > On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: >> On 3/31/2020 4:53 PM, Mark Brown wrote: >>>> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >>>> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); >>> I thought you were going to factor this best guess handling of peak >>> bandwidth out into the core? >> I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) >> but still for QSPI I have to keep this piece of code as is because It is not >> child of QUP wrapper(it doesn't use common code). > Why not? > >> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if >> nothing is mentioned explicitly" to ICC core because the factor of 2 is >> chosen randomly by me. > That's the whole point - if this is just a random number then we may as > well at least be consistently random. Can we centralize below logic of peak_bw selection for all the clients to ICC core? "Assume peak_bw requirement as twice of avg_bw, if it is not mentioned explicitly" =========================================================================== int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw) { struct icc_node *node; u32 old_avg, old_peak; size_t i; int ret; if (!path) return 0; if (WARN_ON(IS_ERR(path) || !path->num_nodes)) return -EINVAL; + /* + * Assume peak_bw requirement as twice of avg_bw, if it is not + * mentioned explicitly + */ + peak_bw = peak_bw ? peak_bw : 2 * avg_bw; =========================================================================== In case if some client really don't want to put peak requirement they can pass avg_bw = peak_bw. As peak_bw <= avg_bw is kind of no-ops. Regards, Akash
Hi Akash, On 4/8/20 15:17, Akash Asthana wrote: > Hi Mark, Evan, Georgi, > > On 4/7/2020 4:25 PM, Mark Brown wrote: >> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: >>> On 3/31/2020 4:53 PM, Mark Brown wrote: >>>>> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >>>>> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); >>>> I thought you were going to factor this best guess handling of peak >>>> bandwidth out into the core? >>> I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) >>> but still for QSPI I have to keep this piece of code as is because It is not >>> child of QUP wrapper(it doesn't use common code). >> Why not? >> >>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if >>> nothing is mentioned explicitly" to ICC core because the factor of 2 is >>> chosen randomly by me. >> That's the whole point - if this is just a random number then we may as >> well at least be consistently random. > > Can we centralize below logic of peak_bw selection for all the clients to ICC core? I don't think this is a good idea for now, because this is very hardware specific. A scaling factor that works for one client might not work for another. My questions here is how did you decide on this "multiply by two"? I can imagine that the traffic can be bursty on some interfaces, but is the factor here really a "random number" or is this based on some data patterns or performance analysis? Thanks, Georgi
On Thu, Apr 09, 2020 at 04:17:22PM +0300, Georgi Djakov wrote: > On 4/8/20 15:17, Akash Asthana wrote: > > Can we centralize below logic of peak_bw selection for all the clients to ICC core? > I don't think this is a good idea for now, because this is very hardware > specific. A scaling factor that works for one client might not work for another. AIUI a driver can always override the setting if it's got a better idea. > My questions here is how did you decide on this "multiply by two"? I can imagine > that the traffic can be bursty on some interfaces, but is the factor here really > a "random number" or is this based on some data patterns or performance > analysis? The reason I'm pushing for this to go into the core is that the numbers seem to be just made up and not device specific at all (or at least there's a lot of devices with the same values).
Hi, On 4/9/20 16:20, Mark Brown wrote: > On Thu, Apr 09, 2020 at 04:17:22PM +0300, Georgi Djakov wrote: >> On 4/8/20 15:17, Akash Asthana wrote: > >>> Can we centralize below logic of peak_bw selection for all the clients to ICC core? > >> I don't think this is a good idea for now, because this is very hardware >> specific. A scaling factor that works for one client might not work for another. > > AIUI a driver can always override the setting if it's got a better idea. True. >> My questions here is how did you decide on this "multiply by two"? I can imagine >> that the traffic can be bursty on some interfaces, but is the factor here really >> a "random number" or is this based on some data patterns or performance >> analysis? > > The reason I'm pushing for this to go into the core is that the numbers > seem to be just made up and not device specific at all (or at least > there's a lot of devices with the same values). I might be a bit too cautious here, but the main question is what this common number should be then. Maybe the numbers are not device specific, but communication interface/bus specific? I would rather make the peak 30% higher than the average, but for low speed interfaces i guess the factor of two should also be fine? Thanks, Georgi
Hi Akash, On 4/10/20 10:31, Akash Asthana wrote: > Hi Georgi, > > On 4/9/2020 6:47 PM, Georgi Djakov wrote: >> Hi Akash, >> >> On 4/8/20 15:17, Akash Asthana wrote: >>> Hi Mark, Evan, Georgi, >>> >>> On 4/7/2020 4:25 PM, Mark Brown wrote: >>>> On Tue, Apr 07, 2020 at 03:24:42PM +0530, Akash Asthana wrote: >>>>> On 3/31/2020 4:53 PM, Mark Brown wrote: >>>>>>> + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); >>>>>>> + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); >>>>>> I thought you were going to factor this best guess handling of peak >>>>>> bandwidth out into the core? >>>>> I can centralize this for SPI, I2C and UART in Common driver(QUP wrapper) >>>>> but still for QSPI I have to keep this piece of code as is because It is not >>>>> child of QUP wrapper(it doesn't use common code). >>>> Why not? >>>> >>>>> I am not sure whether I can move this " Assume peak_bw as twice of avg_bw if >>>>> nothing is mentioned explicitly" to ICC core because the factor of 2 is >>>>> chosen randomly by me. >>>> That's the whole point - if this is just a random number then we may as >>>> well at least be consistently random. >>> Can we centralize below logic of peak_bw selection for all the clients to ICC core? >> I don't think this is a good idea for now, because this is very hardware >> specific. A scaling factor that works for one client might not work for another. >> >> My questions here is how did you decide on this "multiply by two"? I can imagine >> that the traffic can be bursty on some interfaces, but is the factor here really >> a "random number" or is this based on some data patterns or performance >> analysis? > > Factor of 2 is random number. > > 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. Thanks for clarifying, but is this latency a confirmed issue on real hardware? I guess voting for twice as average will work, but still wondering wouldn't it be more appropriate to handle it in the interconnect platform driver instead? Also why is the other client not voting, can we fix it? Thanks, Georgi
diff --git a/drivers/spi/spi-qcom-qspi.c b/drivers/spi/spi-qcom-qspi.c index 3c4f83b..ad48f43 100644 --- a/drivers/spi/spi-qcom-qspi.c +++ b/drivers/spi/spi-qcom-qspi.c @@ -2,6 +2,7 @@ // Copyright (c) 2017-2018, The Linux foundation. All rights reserved. #include <linux/clk.h> +#include <linux/interconnect.h> #include <linux/interrupt.h> #include <linux/io.h> #include <linux/module.h> @@ -139,7 +140,10 @@ struct qcom_qspi { struct device *dev; struct clk_bulk_data *clks; struct qspi_xfer xfer; - /* Lock to protect xfer and IRQ accessed registers */ + struct icc_path *icc_path_cpu_to_qspi; + unsigned int avg_bw_cpu; + unsigned int peak_bw_cpu; + /* Lock to protect data accessed by IRQs */ spinlock_t lock; }; @@ -241,6 +245,20 @@ static int qcom_qspi_transfer_one(struct spi_master *master, return ret; } + /* + * Set BW quota for CPU as driver supports FIFO mode only. + * Assume peak bw as twice of avg bw. + */ + ctrl->avg_bw_cpu = Bps_to_icc(speed_hz); + ctrl->peak_bw_cpu = Bps_to_icc(2 * speed_hz); + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, + ctrl->peak_bw_cpu); + if (ret) { + dev_err(ctrl->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + return ret; + } + spin_lock_irqsave(&ctrl->lock, flags); /* We are half duplex, so either rx or tx will be set */ @@ -458,6 +476,15 @@ static int qcom_qspi_probe(struct platform_device *pdev) if (ret) goto exit_probe_master_put; + ctrl->icc_path_cpu_to_qspi = devm_of_icc_get(dev, "qspi-config"); + if (IS_ERR(ctrl->icc_path_cpu_to_qspi)) { + ret = PTR_ERR(ctrl->icc_path_cpu_to_qspi); + goto exit_probe_master_put; + } + /* Put BW vote on CPU path for register access */ + ctrl->avg_bw_cpu = Bps_to_icc(1000); + ctrl->peak_bw_cpu = Bps_to_icc(1000); + ret = platform_get_irq(pdev, 0); if (ret < 0) goto exit_probe_master_put; @@ -511,9 +538,17 @@ 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); + int ret; clk_bulk_disable_unprepare(QSPI_NUM_CLKS, ctrl->clks); + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, 0, 0); + if (ret) { + dev_err_ratelimited(ctrl->dev, "%s: ICC BW remove failed for cpu\n", + __func__); + return ret; + } + return 0; } @@ -521,6 +556,15 @@ static int __maybe_unused qcom_qspi_runtime_resume(struct device *dev) { struct spi_master *master = dev_get_drvdata(dev); struct qcom_qspi *ctrl = spi_master_get_devdata(master); + int ret; + + ret = icc_set_bw(ctrl->icc_path_cpu_to_qspi, ctrl->avg_bw_cpu, + ctrl->peak_bw_cpu); + if (ret) { + dev_err_ratelimited(ctrl->dev, "%s: ICC BW voting failed for cpu\n", + __func__); + return ret; + } return clk_bulk_prepare_enable(QSPI_NUM_CLKS, ctrl->clks); }
Get the interconnect paths for QSPI device and vote according to the current bus speed of the driver. Signed-off-by: Akash Asthana <akashast@codeaurora.org> --- Changes in V2: - 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: - No Change. drivers/spi/spi-qcom-qspi.c | 46 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)