[v3,5/5] spi: imx: drop useless member speed_hz from driver data struct

Message ID 20181130064709.6998-6-u.kleine-koenig@pengutronix.de
State New
Headers show
Series
  • spi: imx: Fix polarity switching for mx51-ecspi
Related show

Commit Message

Uwe Kleine-König Nov. 30, 2018, 6:47 a.m.
The driver data's member variable just caches the transfer's speed_hz
member. All users of the former now have access directly to the latter.
So fix them to use the uncached value and remove the cache.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/spi/spi-imx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

Comments

Trent Piepho Nov. 30, 2018, 5:21 p.m. | #1
On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> The driver data's member variable just caches the transfer's speed_hz
> member. All users of the former now have access directly to the latter.
> So fix them to use the uncached value and remove the cache.

What should have been done with this is to compare the next transfer's
speed_hz to this cached value, and not preprogram the clock if it has
not changed.  Since usually it doesn't change between transfers.

spi_bus_clk caches the actual spi bus clock, which is often not exactly
the same as the transfer speed, so using it compare against the next
transfer doesn't work as well.
Uwe Kleine-König Nov. 30, 2018, 5:50 p.m. | #2
Hello Trent,

On Fri, Nov 30, 2018 at 05:21:40PM +0000, Trent Piepho wrote:
> On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> > The driver data's member variable just caches the transfer's speed_hz
> > member. All users of the former now have access directly to the latter.
> > So fix them to use the uncached value and remove the cache.
> 
> What should have been done with this is to compare the next transfer's
> speed_hz to this cached value, and not preprogram the clock if it has
> not changed.  Since usually it doesn't change between transfers.

Hmm, yes, this could be done, but I'm not sure the additional effort
brings a performance advantage. If you have the energy to do test that,
feel free to implement it, preferably on top of my cleanup commit.

Best regards
Uwe
Trent Piepho Nov. 30, 2018, 6:10 p.m. | #3
On Fri, 2018-11-30 at 18:50 +0100, Uwe Kleine-König wrote:
> Hello Trent,
> 
> On Fri, Nov 30, 2018 at 05:21:40PM +0000, Trent Piepho wrote:
> > On Fri, 2018-11-30 at 07:47 +0100, Uwe Kleine-König wrote:
> > > The driver data's member variable just caches the transfer's speed_hz
> > > member. All users of the former now have access directly to the latter.
> > > So fix them to use the uncached value and remove the cache.
> > 
> > What should have been done with this is to compare the next transfer's
> > speed_hz to this cached value, and not preprogram the clock if it has
> > not changed.  Since usually it doesn't change between transfers.
> 
> Hmm, yes, this could be done, but I'm not sure the additional effort
> brings a performance advantage. If you have the energy to do test that,
> feel free to implement it, preferably on top of my cleanup commit.

I did this for the imx23 spi driver[1], commit a560943ead, and it more
than double the transfer speed.  That's a much slower CPU than
imx6/7/8, but it could still be a pretty good improvement on something
slow like imx27.

[1] Confusingly, imx23 and imx28 are totally different chips that other
imx chips, like imx21 and imx27.  The former are "mxs" designs that
Freescale bought from someone else (I think ST-Micro?).
Uwe Kleine-König Nov. 30, 2018, 6:21 p.m. | #4
Hello,

On Fri, Nov 30, 2018 at 06:10:26PM +0000, Trent Piepho wrote:
> [1] Confusingly, imx23 and imx28 are totally different chips that other
> imx chips, like imx21 and imx27.  The former are "mxs" designs that
> Freescale bought from someone else (I think ST-Micro?).

FTR: the mxs family includes parts that were bought by Freescale from
Sigmatel.

Best regards
Uwe

Patch

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 72c879226abd..6ec647bbba77 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -87,7 +87,6 @@  struct spi_imx_data {
 	unsigned long spi_clk;
 	unsigned int spi_bus_clk;
 
-	unsigned int speed_hz;
 	unsigned int bits_per_word;
 	unsigned int spi_drctl;
 
@@ -562,7 +561,7 @@  static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 				       struct spi_transfer *t)
 {
 	u32 ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
-	u32 clk = spi_imx->speed_hz, delay;
+	u32 clk = t->speed_hz, delay;
 
 	/* Clear BL field and set the right value */
 	ctrl &= ~MX51_ECSPI_CTRL_BL_MASK;
@@ -576,7 +575,7 @@  static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 	/* set clock speed */
 	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
 		  0xf << MX51_ECSPI_CTRL_PREDIV_OFFSET);
-	ctrl |= mx51_ecspi_clkdiv(spi_imx, spi_imx->speed_hz, &clk);
+	ctrl |= mx51_ecspi_clkdiv(spi_imx, t->speed_hz, &clk);
 	spi_imx->spi_bus_clk = clk;
 
 	if (spi_imx->usedma)
@@ -694,7 +693,7 @@  static int mx31_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, spi_imx->speed_hz, &clk) <<
+	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, t->speed_hz, &clk) <<
 		MX31_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -800,7 +799,7 @@  static int mx21_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int max = is_imx27_cspi(spi_imx) ? 16 : 18;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_1(spi_imx->spi_clk, spi_imx->speed_hz, max, &clk)
+	reg |= spi_imx_clkdiv_1(spi_imx->spi_clk, t->speed_hz, max, &clk)
 		<< MX21_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -875,7 +874,7 @@  static int mx1_prepare_transfer(struct spi_imx_data *spi_imx,
 	unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER;
 	unsigned int clk;
 
-	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, spi_imx->speed_hz, &clk) <<
+	reg |= spi_imx_clkdiv_2(spi_imx->spi_clk, t->speed_hz, &clk) <<
 		MX1_CSPICTRL_DR_SHIFT;
 	spi_imx->spi_bus_clk = clk;
 
@@ -1194,7 +1193,6 @@  static int spi_imx_setupxfer(struct spi_device *spi,
 		return 0;
 
 	spi_imx->bits_per_word = t->bits_per_word;
-	spi_imx->speed_hz = t->speed_hz;
 
 	/*
 	 * Initialize the functions for transfer. To transfer non byte-aligned