| Message ID | 20181130064709.6998-6-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3f75720d0ebb388ff52200d00f2455cc0e6d95a9 |
| Headers | show |
| Series | spi: imx: Fix polarity switching for mx51-ecspi | expand |
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.
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
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?).
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
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
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(-)