Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Mark Brown]

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

On Wed, Jun 28, 2023 at 10:41:27AM +0200, Stefan Moring wrote:

>   ctrl |= (spi_imx->slave_burst * 8 - 1)
>   << MX51_ECSPI_CTRL_BL_OFFSET;
> - else
> - ctrl |= (spi_imx->bits_per_word - 1)
> - << MX51_ECSPI_CTRL_BL_OFFSET;
> + else {
> + if (spi_imx->count >= 512)
> + ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;

Your patch is completely mangled and so can't be applied - please check
your setup for sending things.

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

[PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Stefan Moring]

IMX51 supports 4096 bit burst lengths. Using the spi transfer length
instead of bits_per_word increases performance significantly.

Signed-off-by: Stefan Moring <stefan.moring@technolution.nl>
---
 drivers/spi/spi-imx.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 34e5f81ec431..cbd306c25d28 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -644,9 +644,13 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
 	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
 		ctrl |= (spi_imx->slave_burst * 8 - 1)
 			<< MX51_ECSPI_CTRL_BL_OFFSET;
-	else
-		ctrl |= (spi_imx->bits_per_word - 1)
-			<< MX51_ECSPI_CTRL_BL_OFFSET;
+	else {
+		if (spi_imx->count >= 512)
+			ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
+		else
+			ctrl |= (spi_imx->count*8 - 1)
+				<< MX51_ECSPI_CTRL_BL_OFFSET;
+	}
 
 	/* set clock speed */
 	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
@@ -1243,6 +1247,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		spi_imx->spi_bus_clk = t->speed_hz;
 
 	spi_imx->bits_per_word = t->bits_per_word;
+	spi_imx->count = t->len;
 
 	/*
 	 * Initialize the functions for transfer. To transfer non byte-aligned
-- 
2.34.1

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Stefan Moring]

My bad, I resend it using git send-email.

Op wo 28 jun 2023 om 13:54 schreef Mark Brown <broonie@kernel.org>:
>
> On Wed, Jun 28, 2023 at 10:41:27AM +0200, Stefan Moring wrote:
>
> >   ctrl |= (spi_imx->slave_burst * 8 - 1)
> >   << MX51_ECSPI_CTRL_BL_OFFSET;
> > - else
> > - ctrl |= (spi_imx->bits_per_word - 1)
> > - << MX51_ECSPI_CTRL_BL_OFFSET;
> > + else {
> > + if (spi_imx->count >= 512)
> > + ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
>
> Your patch is completely mangled and so can't be applied - please check
> your setup for sending things.

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Fabio Estevam]

Hi Stefan,

On Wed, Jun 28, 2023 at 9:54 AM Stefan Moring
<stefan.moring@technolution.nl> wrote:
>
> IMX51 supports 4096 bit burst lengths. Using the spi transfer length
> instead of bits_per_word increases performance significantly.

Could you please share the performance increase after this change?

Thanks

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Stefan Moring]

Hi Fabio,

In our application we send ~80kB at 10MHz. The total transfer time
went from ~80ms to 67ms, so that would be a reduction of 15%.
I tested it on an IMX8MM platform.

Kind regards,

Stefan Moring

Op wo 28 jun 2023 om 15:16 schreef Fabio Estevam <festevam@gmail.com>:
>
> Hi Stefan,
>
> On Wed, Jun 28, 2023 at 9:54 AM Stefan Moring
> <stefan.moring@technolution.nl> wrote:
> >
> > IMX51 supports 4096 bit burst lengths. Using the spi transfer length
> > instead of bits_per_word increases performance significantly.
>
> Could you please share the performance increase after this change?
>
> Thanks

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Mark Brown]

On Wed, 28 Jun 2023 14:54:06 +0200, Stefan Moring wrote:
> IMX51 supports 4096 bit burst lengths. Using the spi transfer length
> instead of bits_per_word increases performance significantly.
> 
> 

Applied to

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

Thanks!

[1/1] spi: Increase imx51 ecspi burst length based on transfer length
      commit: 15a6af94a2779d5dfb42ee4bfac858ea8e964a3f

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] spi: Increase imx51 ecspi burst length based on transfer length

[Sebastian Reichel]

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

Hi,

On Wed, Jun 28, 2023 at 02:54:06PM +0200, Stefan Moring wrote:
> IMX51 supports 4096 bit burst lengths. Using the spi transfer length
> instead of bits_per_word increases performance significantly.
> 
> Signed-off-by: Stefan Moring <stefan.moring@technolution.nl>
> ---

I have an i.MX6ULL system with "inanbo,t28cp45tn89-v17" panel, which
breaks due to this change. Reverting this patch results in working
panel. Note, that the panel driver [0] does 'spi->bits_per_word = 9;'.

[0] drivers/gpu/drm/panel/panel-sitronix-st7789v.c

-- Sebastian

>  drivers/spi/spi-imx.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 34e5f81ec431..cbd306c25d28 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -644,9 +644,13 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
>  	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
>  		ctrl |= (spi_imx->slave_burst * 8 - 1)
>  			<< MX51_ECSPI_CTRL_BL_OFFSET;
> -	else
> -		ctrl |= (spi_imx->bits_per_word - 1)
> -			<< MX51_ECSPI_CTRL_BL_OFFSET;
> +	else {
> +		if (spi_imx->count >= 512)
> +			ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
> +		else
> +			ctrl |= (spi_imx->count*8 - 1)
> +				<< MX51_ECSPI_CTRL_BL_OFFSET;
> +	}
>  
>  	/* set clock speed */
>  	ctrl &= ~(0xf << MX51_ECSPI_CTRL_POSTDIV_OFFSET |
> @@ -1243,6 +1247,7 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  		spi_imx->spi_bus_clk = t->speed_hz;
>  
>  	spi_imx->bits_per_word = t->bits_per_word;
> +	spi_imx->count = t->len;
>  
>  	/*
>  	 * Initialize the functions for transfer. To transfer non byte-aligned
> -- 
> 2.34.1
> 

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Sebastian Reichel]

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

Hi,

On Fri, Sep 15, 2023 at 07:55:23AM +0200, Stefan Moring wrote:
> Op vr 15 sep 2023 om 00:47 schreef Sebastian Reichel <sre@kernel.org>:
> > On Wed, Jun 28, 2023 at 02:54:06PM +0200, Stefan Moring wrote:
> > > IMX51 supports 4096 bit burst lengths. Using the spi transfer length
> > > instead of bits_per_word increases performance significantly.
> > >
> > > Signed-off-by: Stefan Moring <stefan.moring@technolution.nl>
> > > ---
> >
> > I have an i.MX6ULL system with "inanbo,t28cp45tn89-v17" panel, which
> > breaks due to this change. Reverting this patch results in working
> > panel. Note, that the panel driver [0] does 'spi->bits_per_word = 9;'.
> >
> > [0] drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>
> Would changing line 665 to ctrl |= (spi_imx->count * spi_imx->bits_per_word
> - 1) fix the issue?

--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -662,7 +662,7 @@ static int mx51_ecspi_prepare_transfer(struct spi_imx_data *spi_imx,
                if (spi_imx->count >= 512)
                        ctrl |= 0xFFF << MX51_ECSPI_CTRL_BL_OFFSET;
                else
-                       ctrl |= (spi_imx->count*8 - 1)
+                       ctrl |= (spi_imx->count*spi_imx->bits_per_word - 1)
                                << MX51_ECSPI_CTRL_BL_OFFSET;
        }

on top of 6.6-rc1 fixes the problem. Will you send a proper patch with

Reported-by: Sebastian Reichel <sre@kernel.org>
Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")

or should I do it?

Greetings,

-- Sebastian

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Marc Kleine-Budde]

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

Hi Stefan,

On 28.06.2023 15:20:39, Stefan Moring wrote:
> In our application we send ~80kB at 10MHz. The total transfer time
> went from ~80ms to 67ms, so that would be a reduction of 15%.
> I tested it on an IMX8MM platform.

I'm currently debugging a problem with spi-imx, HW CS and SPI_CS_WORD on
torvalds/master. The breakage goes back this patch.

I'm wondering what is your setup you have optimized with this patch?
- Are you using HW or GPIO CS?
- What are bits_per_word?
- What's the length of the spi_transfer?

I'm asking because with a 8, 16 or 32 bit-per-word setting, the driver
should use dynamic_burst on the imx8mm, which will overwrite the burst
length in spi_imx_push().

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Marc Kleine-Budde]

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

On 18.06.2024 13:58:07, Marc Kleine-Budde wrote:
> Hi Stefan,
> 
> On 28.06.2023 15:20:39, Stefan Moring wrote:
> > In our application we send ~80kB at 10MHz. The total transfer time
> > went from ~80ms to 67ms, so that would be a reduction of 15%.
> > I tested it on an IMX8MM platform.
> 
> I'm currently debugging a problem with spi-imx, HW CS and SPI_CS_WORD on
> torvalds/master. The breakage goes back this patch.
> 
> I'm wondering what is your setup you have optimized with this patch?
> - Are you using HW or GPIO CS?
> - What are bits_per_word?
> - What's the length of the spi_transfer?
> 
> I'm asking because with a 8, 16 or 32 bit-per-word setting, the driver
> should use dynamic_burst on the imx8mm, which will overwrite the burst
> length in spi_imx_push().

This patch, even with all the fixes on top of it (torvalds/master)
breaks the HW CS + SPI_CS_WORD support which was added in 6e95b23a5b2d
("spi: imx: Implement support for CS_WORD").

I think this also breaks the support for bits-per-word != multiple of 8
bits. For these transfers the in-memory wordsizes are powers of two
bytes (e.g. 20 bit samples use 32 bits) [1] and via the burst length
configuration only the bits-per-word number of bits are shifted out.

[1] https://elixir.bootlin.com/linux/v6.9/source/include/linux/spi/spi.h#L144

| 	if (spi_imx->target_mode && is_imx53_ecspi(spi_imx))
| 		ctrl |= (spi_imx->target_burst * 8 - 1)
| 			<< MX51_ECSPI_CTRL_BL_OFFSET;
| 	else {
| 		if (spi_imx->usedma) {
| 			ctrl |= (spi_imx->bits_per_word - 1)
| 				<< MX51_ECSPI_CTRL_BL_OFFSET;
| 		} else {
| 			if (spi_imx->count >= MX51_ECSPI_CTRL_MAX_BURST)
| 				ctrl |= (MX51_ECSPI_CTRL_MAX_BURST * BITS_PER_BYTE - 1)
| 						<< MX51_ECSPI_CTRL_BL_OFFSET;
| 			else
| 				ctrl |= (spi_imx->count / DIV_ROUND_UP(spi_imx->bits_per_word,
| 						BITS_PER_BYTE) * spi_imx->bits_per_word - 1)
| 						<< MX51_ECSPI_CTRL_BL_OFFSET;
| 		}
| 	}

Consider a message with bits-per-word = 9 consisting of 4 words. It uses
4 word x 2 bytes/word = 8 bytes of memory. This boils down to a burst
length of 36 bits. Which means the spi-imx sends the first 36 bits from
the 64 bits of memory, this is clearly wrong.

This patch (15a6af94a277 ("spi: Increase imx51 ecspi burst length based
on transfer length")) is wrong and the 4 fixes on top of it don't
finally fix it. I can send a series of 5 reverts, or a manually revert
the burst length calculation to the original value in one patch.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Mark Brown]

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

On Tue, Jun 18, 2024 at 05:59:36PM +0200, Marc Kleine-Budde wrote:

> This patch (15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> on transfer length")) is wrong and the 4 fixes on top of it don't
> finally fix it. I can send a series of 5 reverts, or a manually revert
> the burst length calculation to the original value in one patch.

A single revert should be fine and is probably clearer.

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Marc Kleine-Budde]

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

Hey Mark,

On 18.06.2024 17:02:22, Mark Brown wrote:
> On Tue, Jun 18, 2024 at 05:59:36PM +0200, Marc Kleine-Budde wrote:
> 
> > This patch (15a6af94a277 ("spi: Increase imx51 ecspi burst length based
> > on transfer length")) is wrong and the 4 fixes on top of it don't
> > finally fix it. I can send a series of 5 reverts, or a manually revert
> > the burst length calculation to the original value in one patch.
> 
> A single revert should be fine and is probably clearer.

thanks for your quick answer. What about the Fixes-tag? Just the
original patch?

Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

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

Re: [PATCH] spi: Increase imx51 ecspi burst length based on transfer length

[Mark Brown]

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

On Tue, Jun 18, 2024 at 06:06:02PM +0200, Marc Kleine-Budde wrote:
> On 18.06.2024 17:02:22, Mark Brown wrote:

> > A single revert should be fine and is probably clearer.

> thanks for your quick answer. What about the Fixes-tag? Just the
> original patch?

> Fixes: 15a6af94a277 ("spi: Increase imx51 ecspi burst length based on transfer length")

Either way should be fine there, so long as the original patch is
mentioned.  It shouldn't do any harm to mention the others too though.

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