All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Cc: "Michal Simek" <michal.simek@xilinx.com>,
	"Sören Brinkmann" <soren.brinkmann@xilinx.com>,
	linux-spi@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/18] spi-xilinx: Simplify spi_fill_tx_fifo
Date: Mon, 26 Jan 2015 23:54:38 +0000	[thread overview]
Message-ID: <20150126235438.GJ21293@sirena.org.uk> (raw)
In-Reply-To: <1422029330-10971-5-git-send-email-ricardo.ribalda@gmail.com>

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

On Fri, Jan 23, 2015 at 05:08:36PM +0100, Ricardo Ribalda Delgado wrote:
> Instead of checking the TX_FULL flag for every transaction, find out the
> size of the buffer at probe time and use it.

So, I see what's going on here and the potential performance benefit
from avoiding the MMIO access.  However I can't help but worry that this
makes things more fragile - the current code will transparently handle
any races or anything which result in a misfilling of the FIFO for some
reason either now or in the future as performance is improved and the
driver gets more fancy.

As things stand if I look at the code it's fairly clear it should be
safe and unfortunately we only have an underrun interrupt which will be
triggered normally (no overflow interrupt) so we can't really use that
to add a bit of robustness.  I'm tempted to suggest checking that we did
trigger the FIFO full flag when we fill the FIFO but that feels like
overengineering.

Let me think about this, I'll probably apply it but if you can think
about ways of making this more robust that'd be good.

> @@ -413,6 +424,8 @@ static int xilinx_spi_probe(struct platform_device *pdev)
>  		goto put_master;
>  	}
>  
> +	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
> +
>  	/* SPI controller initializations */
>  	xspi_init_hw(xspi);

It seems safer to reset the hardware, probe the FIFO size and then reset
the hardware again in case something decided to leave some data in the
FIFO (perhaps some bootloader wasn't as well programmed as one might
desire for example).  Not cricial now but it could again become
important with future optimizations.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

WARNING: multiple messages have this Message-ID (diff)
From: broonie@kernel.org (Mark Brown)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 04/18] spi-xilinx: Simplify spi_fill_tx_fifo
Date: Mon, 26 Jan 2015 23:54:38 +0000	[thread overview]
Message-ID: <20150126235438.GJ21293@sirena.org.uk> (raw)
In-Reply-To: <1422029330-10971-5-git-send-email-ricardo.ribalda@gmail.com>

On Fri, Jan 23, 2015 at 05:08:36PM +0100, Ricardo Ribalda Delgado wrote:
> Instead of checking the TX_FULL flag for every transaction, find out the
> size of the buffer at probe time and use it.

So, I see what's going on here and the potential performance benefit
from avoiding the MMIO access.  However I can't help but worry that this
makes things more fragile - the current code will transparently handle
any races or anything which result in a misfilling of the FIFO for some
reason either now or in the future as performance is improved and the
driver gets more fancy.

As things stand if I look at the code it's fairly clear it should be
safe and unfortunately we only have an underrun interrupt which will be
triggered normally (no overflow interrupt) so we can't really use that
to add a bit of robustness.  I'm tempted to suggest checking that we did
trigger the FIFO full flag when we fill the FIFO but that feels like
overengineering.

Let me think about this, I'll probably apply it but if you can think
about ways of making this more robust that'd be good.

> @@ -413,6 +424,8 @@ static int xilinx_spi_probe(struct platform_device *pdev)
>  		goto put_master;
>  	}
>  
> +	xspi->buffer_size = xilinx_spi_find_buffer_size(xspi);
> +
>  	/* SPI controller initializations */
>  	xspi_init_hw(xspi);

It seems safer to reset the hardware, probe the FIFO size and then reset
the hardware again in case something decided to leave some data in the
FIFO (perhaps some bootloader wasn't as well programmed as one might
desire for example).  Not cricial now but it could again become
important with future optimizations.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150126/905f07e9/attachment.sig>

  reply	other threads:[~2015-01-26 23:55 UTC|newest]

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-23 16:08 [PATCH 00/18] spi/xilinx: Speed-up Ricardo Ribalda Delgado
2015-01-23 16:08 ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 01/18] spi/xilinx: Support for spi mode LSB_FIRST Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-26 19:23   ` Mark Brown
2015-01-26 19:23     ` Mark Brown
2015-01-26 19:23     ` Mark Brown
2015-01-23 16:08 ` [PATCH 02/18] spi/xilinx: Support for spi mode LOOP Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-26 19:23   ` Mark Brown
2015-01-26 19:23     ` Mark Brown
2015-01-26 19:23     ` Mark Brown
2015-01-23 16:08 ` [PATCH 03/18] spi/xilinx: Simplify data read from the Rx FIFO Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27 17:25   ` Mark Brown
2015-01-27 17:25     ` Mark Brown
2015-01-27 17:25     ` Mark Brown
2015-01-23 16:08 ` [PATCH 04/18] spi-xilinx: Simplify spi_fill_tx_fifo Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-26 23:54   ` Mark Brown [this message]
2015-01-26 23:54     ` Mark Brown
2015-01-23 16:08 ` [PATCH 05/18] spi/xilinx: Leave the IRQ always enabled Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 06/18] spi/xilinx: Code cleanup Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27  0:05   ` Mark Brown
2015-01-27  0:05     ` Mark Brown
2015-01-27  0:05     ` Mark Brown
2015-01-23 16:08 ` [PATCH 07/18] spi/xilinx: Use cached value of register Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 08/18] spi/xilinx: Support cores with no interrupt Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27  0:04   ` Mark Brown
2015-01-27  0:04     ` Mark Brown
2015-01-27  0:04     ` Mark Brown
2015-01-27 19:05     ` Ricardo Ribalda Delgado
2015-01-27 19:05       ` Ricardo Ribalda Delgado
2015-01-27 19:49       ` Mark Brown
2015-01-27 19:49         ` Mark Brown
2015-01-27 19:49         ` Mark Brown
2015-01-27 19:56         ` Ricardo Ribalda Delgado
2015-01-27 19:56           ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 09/18] spi/xilinx: Do not inhibit transmission in polling mode Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 10/18] spi/xilinx: Support for spi mode CS_HIGH Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 11/18] spi/xilinx: Remove rx_fn and tx_fn pointer Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27  0:09   ` Mark Brown
2015-01-27  0:09     ` Mark Brown
2015-01-27  0:09     ` Mark Brown
2015-01-27 10:00     ` Ricardo Ribalda Delgado
2015-01-27 10:00       ` Ricardo Ribalda Delgado
2015-01-27 10:00       ` Ricardo Ribalda Delgado
2015-01-27 15:42       ` Joe Perches
2015-01-27 15:42         ` Joe Perches
2015-01-27 15:42         ` Joe Perches
2015-01-27 19:07         ` Ricardo Ribalda Delgado
2015-01-27 19:07           ` Ricardo Ribalda Delgado
2015-01-27 19:07           ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 12/18] spi/xilinx: Make spi_tx and spi_rx simmetric Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 13/18] spi/xilinx: Convert remainding_bytes in remaining words Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 14/18] spi/xilinx: Convert bits_per_word in bytes_per_word Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 15/18] spi/xilinx: Remove iowrite/ioread wrappers Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 16/18] spi/xilinx: Reset core before buffer_size detection Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27  0:11   ` Mark Brown
2015-01-27  0:11     ` Mark Brown
2015-01-27  0:11     ` Mark Brown
2015-01-23 16:08 ` [PATCH 17/18] spi/xilinx: Remove remaining_words driver data variable Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-23 16:08 ` [PATCH 18/18] spi/xilinx: Check number of slaves range Ricardo Ribalda Delgado
2015-01-23 16:08   ` Ricardo Ribalda Delgado
2015-01-27  0:14 ` [PATCH 00/18] spi/xilinx: Speed-up Mark Brown
2015-01-27  0:14   ` Mark Brown
2015-01-27  0:14   ` Mark Brown
2015-01-27 10:17   ` Ricardo Ribalda Delgado
2015-01-27 10:17     ` Ricardo Ribalda Delgado
2015-01-27 11:59     ` Mark Brown
2015-01-27 11:59       ` Mark Brown
2015-01-27 11:59       ` Mark Brown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150126235438.GJ21293@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-spi@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=ricardo.ribalda@gmail.com \
    --cc=soren.brinkmann@xilinx.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.