Re: [PATCH linux-next v3 1/1] spi: imx: dynamic burst length adjust for PIO mode

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Jun 08, 2017 at 10:09:07PM -0700, Jiada Wang wrote:
> On 06/01/2017 10:38 PM, Sascha Hauer wrote:
> > On Thu, Jun 01, 2017 at 04:58:47AM -0700, Jiada Wang wrote:
> > > Hi Sascha
> > > 
> > > On 05/29/2017 02:50 AM, Sascha Hauer wrote:
> > > > Hi,
> > > > 
> > > > On Thu, May 25, 2017 at 10:02:42PM -0700, jiada_wang@mentor.com wrote:
> > > > > From: Jiada Wang<jiada_wang@mentor.com>
> > > > > 
> > > > > previously burst length (BURST_LENGTH) is always set to equal
> > > > > to bits_per_word, causes a 10us gap between each word in
> > > > > transfer, which significantly affects performance.
> > > > > 
> > > > > This patch uses 32 bits transfer to simulate lower bits transfer,
> > > > > and adjusts burst length runtimely to use biggeest burst length
> > > > > as possible to reduce the gaps in transfer for PIO mode.
> > > > > 
> > > > First let me say that I'm not really looking forward to have this patch
> > > > in the driver. It adds quite some code to already hairy code pathes in
> > > > the imx-spi driver and I saw you have the same patch for DMA mode
> > > > aswell.
> > > > 
> > > > The driver has different function hooks for the different controllers.
> > > > This patch breaks that. In some places it assumes that dynamic burst
> > > > is only possible on i.MX51 type controllers and also that in case
> > > > dynamic burst is enabled it must be an i.MX51 type controller.
> > > > 
> > > > We should really see how this patch can be better integrated into the
> > > > driver, or, how the driver can be changed to better support the dynamic
> > > > burst usecase.
> > >   Yes, I can understand your concern, as this patch brings in a bunch of
> > > change,
> > > and changes the behaviour of data transfer.
> > > how about introduce a new DTS property like "fsl,spi-dynamic-burst",
> > > and only enables dynamic burst when this property is added.
> > This will only reduce the testing coverage of the feature, not a good
> > option.
> As an improvement, I am thinking about the following
> 1. introduce a 'fsl,spi-dynamic-burst' property in dts as mentioned before
> 2. remove all change in specific functions hooks, for example
> "mx51_ecspi_config"
> 3. introduce spi_imx_pio_dynamic_burst_transfer(), which will handle actual
> swapped data transfer,
>     and burst length adjustment.

Whether or not the spi driver uses dynamic bursts is an implementation
detail of the driver and should be decided by the driver, not by the
device tree.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |