[PATCH 0/3] spi: spi-geni-qcom: Avoid a bunch of per-transfer overhead

[PATCH 0/3] spi: spi-geni-qcom: Avoid a bunch of per-transfer overhead

[Douglas Anderson]

This series tries to reduce a whole bunch of overhead in each SPI
transfer.  Much of this overhead is new with the recent interconnect
changes, but even without those changes we still had some overhead
that we could avoid.  Let's avoid all of it.

These changes are atop the Qualcomm tree to avoid merge conflicts.  If
they look good, the most expedient way to land them is probably to get
Ack's from Mark and land then via the Qualcomm tree.

Most testing was done on the Chrome OS 5.4 tree, but sanity check was
done on mainline.


Douglas Anderson (3):
  spi: spi-geni-qcom: Avoid clock setting if not needed
  spi: spi-geni-qcom: Set an autosuspend delay of 250 ms
  spi: spi-geni-qcom: Get rid of most overhead in prepare_message()

 drivers/spi/spi-geni-qcom.c | 67 ++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 35 deletions(-)

-- 
2.27.0.383.g050319c2ae-goog

[PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Douglas Anderson]

Every SPI transfer could have a different clock rate.  The
spi-geni-qcom controller code to deal with this was never very well
optimized and has always had a lot of code plus some calls into the
clk framework which, at the very least, would grab a mutex.  However,
until recently, the overhead wasn't _too_ much.  That changed with
commit 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect support")
we're now calling geni_icc_set_bw(), which leads to a bunch of math
plus:
  geni_icc_set_bw()
    icc_set_bw()
      apply_constraints()
        qcom_icc_set()
          qcom_icc_bcm_voter_commit()
            rpmh_invalidate()
            rpmh_write_batch()
...and those rpmh commands can be a bit beefy if you call them too
often.

We already know what speed we were running at before, so if we see
that nothing has changed let's avoid the whole pile of code.

On my hardware, this made spi_geni_prepare_message() drop down from
~145 us down to ~14 us.

NOTE: Potentially it might also make sense to add some code into the
interconnect framework to avoid executing so much code when bandwidth
isn't changing, but even if we did that we still want to short circuit
here to save the extra math / clock calls.

Fixes: 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi-geni-qcom.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index e01c782ef7d0..bb4cdda2dec8 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -201,6 +201,9 @@ static int geni_spi_set_clock_and_bw(struct spi_geni_master *mas,
 	struct geni_se *se = &mas->se;
 	int ret;
 
+	if (clk_hz == mas->cur_speed_hz)
+		return 0;
+
 	ret = get_spi_clk_cfg(clk_hz, mas, &idx, &div);
 	if (ret) {
 		dev_err(mas->dev, "Err setting clk to %lu: %d\n", clk_hz, ret);
@@ -339,11 +342,9 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	}
 
 	/* Speed and bits per word can be overridden per transfer */
-	if (xfer->speed_hz != mas->cur_speed_hz) {
-		ret = geni_spi_set_clock_and_bw(mas, xfer->speed_hz);
-		if (ret)
-			return;
-	}
+	ret = geni_spi_set_clock_and_bw(mas, xfer->speed_hz);
+	if (ret)
+		return;
 
 	mas->tx_rem_bytes = 0;
 	mas->rx_rem_bytes = 0;
-- 
2.27.0.383.g050319c2ae-goog

[PATCH 2/3] spi: spi-geni-qcom: Set an autosuspend delay of 250 ms

[Douglas Anderson]

In commit 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect
support") the spi_geni_runtime_suspend() and spi_geni_runtime_resume()
became a bit slower.  Measuring on my hardware I see numbers in the
hundreds of microseconds now.

Let's use autosuspend to help avoid some of the overhead.  Now if
we're doing a bunch of transfers we won't need to be constantly
chruning.

The number 250 ms for the autosuspend delay was picked a bit
arbitrarily, so if someone has measurements showing a better value we
could easily change this.

Fixes: 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect support")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi-geni-qcom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index bb4cdda2dec8..f51279608fc7 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -595,6 +595,8 @@ static int spi_geni_probe(struct platform_device *pdev)
 
 	init_completion(&mas->xfer_done);
 	spin_lock_init(&mas->lock);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
 	pm_runtime_enable(dev);
 
 	ret = geni_icc_get(&mas->se, NULL);
-- 
2.27.0.383.g050319c2ae-goog

[PATCH 3/3] spi: spi-geni-qcom: Get rid of most overhead in prepare_message()

[Douglas Anderson]

There's a bunch of overhead in spi-geni-qcom's prepare_message.  Get
rid of it.  Before this change spi_geni_prepare_message() took around
14.5 us.  After this change, spi_geni_prepare_message() takes about
1.75 us (as measured by ftrace).

What's here:
* We're always in FIFO mode, so no need to call it for every transfer.
  This avoids a whole ton of readl/writel calls.
* We don't need to write a whole pile of config registers if the mode
  isn't changing.  Cache the last mode and only do the work if needed.
* For several registers we were trying to do read/modify/write, but
  there was no reason.  The registers only have one thing in them, so
  just write them.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/spi/spi-geni-qcom.c | 54 +++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index f51279608fc7..97fac5ea6afd 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -77,6 +77,7 @@ struct spi_geni_master {
 	u32 tx_fifo_depth;
 	u32 fifo_width_bits;
 	u32 tx_wm;
+	u32 last_mode;
 	unsigned long cur_speed_hz;
 	unsigned int cur_bits_per_word;
 	unsigned int tx_rem_bytes;
@@ -177,8 +178,6 @@ static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
 	struct geni_se *se = &mas->se;
 	u32 word_len;
 
-	word_len = readl(se->base + SE_SPI_WORD_LEN);
-
 	/*
 	 * If bits_per_word isn't a byte aligned value, set the packing to be
 	 * 1 SPI word per FIFO word.
@@ -187,10 +186,9 @@ static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
 		pack_words = mas->fifo_width_bits / bits_per_word;
 	else
 		pack_words = 1;
-	word_len &= ~WORD_LEN_MSK;
-	word_len |= ((bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK);
 	geni_se_config_packing(&mas->se, bits_per_word, pack_words, msb_first,
 								true, true);
+	word_len = (bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK;
 	writel(word_len, se->base + SE_SPI_WORD_LEN);
 }
 
@@ -238,38 +236,34 @@ static int setup_fifo_params(struct spi_device *spi_slv,
 {
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
 	struct geni_se *se = &mas->se;
-	u32 loopback_cfg, cpol, cpha, demux_output_inv;
+	u32 loopback_cfg = 0, cpol = 0, cpha = 0, demux_output_inv = 0;
 	u32 demux_sel;
 
-	loopback_cfg = readl(se->base + SE_SPI_LOOPBACK);
-	cpol = readl(se->base + SE_SPI_CPOL);
-	cpha = readl(se->base + SE_SPI_CPHA);
-	demux_output_inv = 0;
-	loopback_cfg &= ~LOOPBACK_MSK;
-	cpol &= ~CPOL;
-	cpha &= ~CPHA;
+	if (mas->last_mode != spi_slv->mode) {
+		if (spi_slv->mode & SPI_LOOP)
+			loopback_cfg = LOOPBACK_ENABLE;
 
-	if (spi_slv->mode & SPI_LOOP)
-		loopback_cfg |= LOOPBACK_ENABLE;
+		if (spi_slv->mode & SPI_CPOL)
+			cpol = CPOL;
 
-	if (spi_slv->mode & SPI_CPOL)
-		cpol |= CPOL;
+		if (spi_slv->mode & SPI_CPHA)
+			cpha = CPHA;
 
-	if (spi_slv->mode & SPI_CPHA)
-		cpha |= CPHA;
+		if (spi_slv->mode & SPI_CS_HIGH)
+			demux_output_inv = BIT(spi_slv->chip_select);
 
-	if (spi_slv->mode & SPI_CS_HIGH)
-		demux_output_inv = BIT(spi_slv->chip_select);
+		demux_sel = spi_slv->chip_select;
+		mas->cur_bits_per_word = spi_slv->bits_per_word;
 
-	demux_sel = spi_slv->chip_select;
-	mas->cur_bits_per_word = spi_slv->bits_per_word;
+		spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
+		writel(loopback_cfg, se->base + SE_SPI_LOOPBACK);
+		writel(demux_sel, se->base + SE_SPI_DEMUX_SEL);
+		writel(cpha, se->base + SE_SPI_CPHA);
+		writel(cpol, se->base + SE_SPI_CPOL);
+		writel(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
 
-	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
-	writel(loopback_cfg, se->base + SE_SPI_LOOPBACK);
-	writel(demux_sel, se->base + SE_SPI_DEMUX_SEL);
-	writel(cpha, se->base + SE_SPI_CPHA);
-	writel(cpol, se->base + SE_SPI_CPOL);
-	writel(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
+		mas->last_mode = spi_slv->mode;
+	}
 
 	return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz);
 }
@@ -279,9 +273,7 @@ static int spi_geni_prepare_message(struct spi_master *spi,
 {
 	int ret;
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
-	struct geni_se *se = &mas->se;
 
-	geni_se_select_mode(se, GENI_SE_FIFO);
 	ret = setup_fifo_params(spi_msg->spi, spi);
 	if (ret)
 		dev_err(mas->dev, "Couldn't select mode %d\n", ret);
@@ -322,6 +314,8 @@ static int spi_geni_init(struct spi_geni_master *mas)
 	else
 		mas->oversampling = 1;
 
+	geni_se_select_mode(se, GENI_SE_FIFO);
+
 	pm_runtime_put(mas->dev);
 	return 0;
 }
-- 
2.27.0.383.g050319c2ae-goog

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Akash Asthana]

On 7/2/2020 6:15 AM, Douglas Anderson wrote:
> Every SPI transfer could have a different clock rate.  The
> spi-geni-qcom controller code to deal with this was never very well
> optimized and has always had a lot of code plus some calls into the
> clk framework which, at the very least, would grab a mutex.  However,
> until recently, the overhead wasn't _too_ much.  That changed with
> commit 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect support")
> we're now calling geni_icc_set_bw(), which leads to a bunch of math
> plus:
>    geni_icc_set_bw()
>      icc_set_bw()
>        apply_constraints()
>          qcom_icc_set()
>            qcom_icc_bcm_voter_commit()
>              rpmh_invalidate()
>              rpmh_write_batch()
> ...and those rpmh commands can be a bit beefy if you call them too
> often.

Reviewed-by: Akash Asthana<akashast@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH 2/3] spi: spi-geni-qcom: Set an autosuspend delay of 250 ms

[Akash Asthana]

On 7/2/2020 6:15 AM, Douglas Anderson wrote:
> In commit 0e3b8a81f5df ("spi: spi-geni-qcom: Add interconnect
> support") the spi_geni_runtime_suspend() and spi_geni_runtime_resume()
> became a bit slower.  Measuring on my hardware I see numbers in the
> hundreds of microseconds now.
>
> Let's use autosuspend to help avoid some of the overhead.  Now if
> we're doing a bunch of transfers we won't need to be constantly
> chruning.
>
> The number 250 ms for the autosuspend delay was picked a bit
> arbitrarily, so if someone has measurements showing a better value we
> could easily change this.

Reviewed-by: Akash Asthana<akashast@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

On Wed, Jul 01, 2020 at 05:45:07PM -0700, Douglas Anderson wrote:
> Every SPI transfer could have a different clock rate.  The
> spi-geni-qcom controller code to deal with this was never very well
> optimized and has always had a lot of code plus some calls into the

This doesn't apply against current code, please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Doug Anderson]

Hi Mark,

On Tue, Jul 7, 2020 at 5:08 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Jul 01, 2020 at 05:45:07PM -0700, Douglas Anderson wrote:
> > Every SPI transfer could have a different clock rate.  The
> > spi-geni-qcom controller code to deal with this was never very well
> > optimized and has always had a lot of code plus some calls into the
>
> This doesn't apply against current code, please check and resend.

As mentioned in the cover letter, I posted this series against the
Qualcomm tree.  The commit that it is fixing landed there with your
Ack so I was hoping this series could land in the Qualcomm tree with
your Ack as well.  Would that be OK?

-Doug

Re: [PATCH 3/3] spi: spi-geni-qcom: Get rid of most overhead in prepare_message()

[Akash Asthana]

On 7/2/2020 6:15 AM, Douglas Anderson wrote:
> There's a bunch of overhead in spi-geni-qcom's prepare_message.  Get
> rid of it.  Before this change spi_geni_prepare_message() took around
> 14.5 us.  After this change, spi_geni_prepare_message() takes about
> 1.75 us (as measured by ftrace).
>
> What's here:
> * We're always in FIFO mode, so no need to call it for every transfer.
>    This avoids a whole ton of readl/writel calls.
> * We don't need to write a whole pile of config registers if the mode
>    isn't changing.  Cache the last mode and only do the work if needed.
> * For several registers we were trying to do read/modify/write, but
>    there was no reason.  The registers only have one thing in them, so
>    just write them.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>   drivers/spi/spi-geni-qcom.c | 54 +++++++++++++++++--------------------
>   1 file changed, 24 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index f51279608fc7..97fac5ea6afd 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -77,6 +77,7 @@ struct spi_geni_master {
>   	u32 tx_fifo_depth;
>   	u32 fifo_width_bits;
>   	u32 tx_wm;
> +	u32 last_mode;
>   	unsigned long cur_speed_hz;
>   	unsigned int cur_bits_per_word;
>   	unsigned int tx_rem_bytes;
> @@ -177,8 +178,6 @@ static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
>   	struct geni_se *se = &mas->se;
>   	u32 word_len;
>   
> -	word_len = readl(se->base + SE_SPI_WORD_LEN);
> -
>   	/*
>   	 * If bits_per_word isn't a byte aligned value, set the packing to be
>   	 * 1 SPI word per FIFO word.
> @@ -187,10 +186,9 @@ static void spi_setup_word_len(struct spi_geni_master *mas, u16 mode,
>   		pack_words = mas->fifo_width_bits / bits_per_word;
>   	else
>   		pack_words = 1;
> -	word_len &= ~WORD_LEN_MSK;
> -	word_len |= ((bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK);
>   	geni_se_config_packing(&mas->se, bits_per_word, pack_words, msb_first,
>   								true, true);
> +	word_len = (bits_per_word - MIN_WORD_LEN) & WORD_LEN_MSK;
>   	writel(word_len, se->base + SE_SPI_WORD_LEN);
>   }
>   
> @@ -238,38 +236,34 @@ static int setup_fifo_params(struct spi_device *spi_slv,
>   {
>   	struct spi_geni_master *mas = spi_master_get_devdata(spi);
>   	struct geni_se *se = &mas->se;
> -	u32 loopback_cfg, cpol, cpha, demux_output_inv;
> +	u32 loopback_cfg = 0, cpol = 0, cpha = 0, demux_output_inv = 0;
>   	u32 demux_sel;
>   
> -	loopback_cfg = readl(se->base + SE_SPI_LOOPBACK);
> -	cpol = readl(se->base + SE_SPI_CPOL);
> -	cpha = readl(se->base + SE_SPI_CPHA);
> -	demux_output_inv = 0;
> -	loopback_cfg &= ~LOOPBACK_MSK;
> -	cpol &= ~CPOL;
> -	cpha &= ~CPHA;
> +	if (mas->last_mode != spi_slv->mode) {
> +		if (spi_slv->mode & SPI_LOOP)
> +			loopback_cfg = LOOPBACK_ENABLE;
>   
> -	if (spi_slv->mode & SPI_LOOP)
> -		loopback_cfg |= LOOPBACK_ENABLE;
> +		if (spi_slv->mode & SPI_CPOL)
> +			cpol = CPOL;
>   
> -	if (spi_slv->mode & SPI_CPOL)
> -		cpol |= CPOL;
> +		if (spi_slv->mode & SPI_CPHA)
> +			cpha = CPHA;
>   
> -	if (spi_slv->mode & SPI_CPHA)
> -		cpha |= CPHA;
> +		if (spi_slv->mode & SPI_CS_HIGH)
> +			demux_output_inv = BIT(spi_slv->chip_select);
>   
> -	if (spi_slv->mode & SPI_CS_HIGH)
> -		demux_output_inv = BIT(spi_slv->chip_select);
> +		demux_sel = spi_slv->chip_select;
> +		mas->cur_bits_per_word = spi_slv->bits_per_word;
>   
> -	demux_sel = spi_slv->chip_select;
> -	mas->cur_bits_per_word = spi_slv->bits_per_word;
> +		spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
> +		writel(loopback_cfg, se->base + SE_SPI_LOOPBACK);
> +		writel(demux_sel, se->base + SE_SPI_DEMUX_SEL);
> +		writel(cpha, se->base + SE_SPI_CPHA);
> +		writel(cpol, se->base + SE_SPI_CPOL);
> +		writel(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
>   
> -	spi_setup_word_len(mas, spi_slv->mode, spi_slv->bits_per_word);
> -	writel(loopback_cfg, se->base + SE_SPI_LOOPBACK);
> -	writel(demux_sel, se->base + SE_SPI_DEMUX_SEL);
> -	writel(cpha, se->base + SE_SPI_CPHA);
> -	writel(cpol, se->base + SE_SPI_CPOL);
> -	writel(demux_output_inv, se->base + SE_SPI_DEMUX_OUTPUT_INV);
> +		mas->last_mode = spi_slv->mode;
> +	}
>   
>   	return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz);
>   }

Yeah looks good to me, the default/reset value of these registers are 0 
we don't have to preserve any bits here.

We can directly update the register with required value.

Reviewed-by: Akash Asthana <akashast@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,\na Linux Foundation Collaborative Project

Re: [PATCH 0/3] spi: spi-geni-qcom: Avoid a bunch of per-transfer overhead

[Mark Brown]

On Wed, 1 Jul 2020 17:45:06 -0700, Douglas Anderson wrote:
> This series tries to reduce a whole bunch of overhead in each SPI
> transfer.  Much of this overhead is new with the recent interconnect
> changes, but even without those changes we still had some overhead
> that we could avoid.  Let's avoid all of it.
> 
> These changes are atop the Qualcomm tree to avoid merge conflicts.  If
> they look good, the most expedient way to land them is probably to get
> Ack's from Mark and land then via the Qualcomm tree.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/3] spi: spi-geni-qcom: Avoid clock setting if not needed
      (no commit info)
[2/3] spi: spi-geni-qcom: Set an autosuspend delay of 250 ms
      commit: e99f0b6ef2679b0abeefcd7bd148cd65651c7857
[3/3] spi: spi-geni-qcom: Get rid of most overhead in prepare_message()
      (no commit info)

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Tue, Jul 07, 2020 at 05:53:01AM -0700, Doug Anderson wrote:
> On Tue, Jul 7, 2020 at 5:08 AM Mark Brown <broonie@kernel.org> wrote:

> > This doesn't apply against current code, please check and resend.

> As mentioned in the cover letter, I posted this series against the
> Qualcomm tree.  The commit that it is fixing landed there with your
> Ack so I was hoping this series could land in the Qualcomm tree with
> your Ack as well.  Would that be OK?

So I didn't see this until after the patch I applied was queued...  it's
looking like it would be good to have a cross-tree merge with the
Qualcomm tree if there's stuff like this - is this on a branch which
makes that practical?  Otherwise I guess...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On Wed, Jul 01, 2020 at 05:45:07PM -0700, Douglas Anderson wrote:
> Every SPI transfer could have a different clock rate.  The
> spi-geni-qcom controller code to deal with this was never very well
> optimized and has always had a lot of code plus some calls into the

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] spi: spi-geni-qcom: Get rid of most overhead in prepare_message()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On Wed, Jul 01, 2020 at 05:45:09PM -0700, Douglas Anderson wrote:
> There's a bunch of overhead in spi-geni-qcom's prepare_message.  Get
> rid of it.  Before this change spi_geni_prepare_message() took around
> 14.5 us.  After this change, spi_geni_prepare_message() takes about
> 1.75 us (as measured by ftrace).

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Doug Anderson]

Hi,

On Wed, Jul 8, 2020 at 3:01 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 07, 2020 at 05:53:01AM -0700, Doug Anderson wrote:
> > On Tue, Jul 7, 2020 at 5:08 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > This doesn't apply against current code, please check and resend.
>
> > As mentioned in the cover letter, I posted this series against the
> > Qualcomm tree.  The commit that it is fixing landed there with your
> > Ack so I was hoping this series could land in the Qualcomm tree with
> > your Ack as well.  Would that be OK?
>
> So I didn't see this until after the patch I applied was queued...  it's
> looking like it would be good to have a cross-tree merge with the
> Qualcomm tree if there's stuff like this - is this on a branch which
> makes that practical?  Otherwise I guess...

It's not too bad.  Of the 5 patches I've sent out (3 for geni SPI, 2
for quad SPI) you've landed just one.  Here's the summary:

a) geni SPI 1/3 (Avoid clock setting): Has your Ack.
b) geni SPI 2/3 (autosuspend delay): Landed in SPI tree
c) geni SPI 3/3 (overhead in prepare_message): Has your Ack.

d) quad SPI 1/2 (Avoid clock setting): Needs your Ack.
e) quad SPI 2/2 (autosuspend delay): Needs your Ack.

Since b) has already landed in your tree, let's just leave it there.
There'll be a bit of a performance hit in the Qualcomm tree, but it'll
still be usable.

Since the rest haven't landed, it would be nice to just land them in
the Qualcomm tree.


I think there's still more work to make the Geni SPI driver more
optimized, but I don't think it'll be as urgent as those patches and I
feel like any more major work could wait a cycle.


-Doug

Re: [PATCH 1/3] spi: spi-geni-qcom: Avoid clock setting if not needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Wed, Jul 08, 2020 at 08:22:05AM -0700, Doug Anderson wrote:

> Since the rest haven't landed, it would be nice to just land them in
> the Qualcomm tree.

I guess.

> I think there's still more work to make the Geni SPI driver more
> optimized, but I don't think it'll be as urgent as those patches and I
> feel like any more major work could wait a cycle.

It feels like there's more than what's already landed in flight at the
minute, though some of that might just be the multiple rounds of reviews
there were for the bandwidth stuff.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]