Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1191141 > unrolled thread

Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-23 19:10 +0200
    Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Marek Vasut <marex@denx.de> - 2015-07-24 11:20 +0200
      Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-24 13:30 +0200
      Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size. Michal Suchanek <hramrach@gmail.com> - 2015-07-27 11:50 +0200

#1191141 — Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.

On 23 July 2015 at 18:46, Michal Suchanek <hramrach@gmail.com> wrote:
> On 22 July 2015 at 11:01, Marek Vasut <marex@denx.de> wrote:
>> On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote:
>>> On 22 July 2015 at 10:24, Marek Vasut <marex@denx.de> wrote:
>>> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
>>> >> On 22 July 2015 at 09:58, Marek Vasut <marex@denx.de> wrote:
>>> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>>> >> >> On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
>>> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>>> >> >> >> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
>>> >> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>>> >> >> >> >> > Or alternatively we could publish the limitations of the
>>> >> >> >> >> > channel using capabilities so SPI knows I have a dmaengine
>>> >> >> >> >> > channel and it can transfer max N length transfers so would
>>> >> >> >> >> > be able to break rather than guessing it or coding in DT.
>>> >> >> >> >> > Yes it may come from DT but that should be dmaengine driver
>>> >> >> >> >> > rather than client driver :)
>>> >> >> >> >> >
>>> >> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps)
>>> >> >> >> >> >
>>> >> >> >> >> > And we add max_length as one more parameter to existing set
>>> >> >> >> >> >
>>> >> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer
>>> >> >> >> >> > so that individual drivers don't have to code it in
>>> >> >> >> >> >
>>> >> >> >> >> > Let me know if this idea is okay, I can push the dmaengine
>>> >> >> >> >> > bits...
>>> >> >> >> >>
>>> >> >> >> >> It would be ok if there was a fixed limit. However, the limit
>>> >> >> >> >> depends on SPI slave settings. Presumably for other buses using
>>> >> >> >> >> the dmaengine the limit would depend on the bus or slave
>>> >> >> >> >> settings as well. I do not see a sane way of passing this all
>>> >> >> >> >> the way to the dmaengine driver.
>>> >> >> >> >
>>> >> >> >> > I don't see why this should be client (SPI) dependent. The max
>>> >> >> >> > length supported is a dmaengine constraint, typically flowing
>>> >> >> >> > from max blocks/length it can transfer. Know this limit can
>>> >> >> >> > allow clients to split transfers.
>>> >> >> >>
>>> >> >> >> In practice on the board I have the maximum transfer length before
>>> >> >> >> it fails depends on SPI bus speed which is set up per slave. I
>>> >> >> >> did not try searching the space of possible settings thorougly
>>> >> >> >> and settled for a setting that gives reasonable speed and
>>> >> >> >> transfer length.
>>> >> >> >
>>> >> >> > This looks more like a signal integrity issue though.
>>> >> >>
>>> >> >> It certainly does on the surface. However, when wrong data is
>>> >> >> delivered over the SPI bus (such as when I use wrong phase setting)
>>> >> >> the SPI controller happily delivers wrong data over PIO.
>>> >> >>
>>> >> >> The failure I am seeing is that the pl330 DMA program which
>>> >> >> repeatedly waits for data from the SPI controller never finishes the
>>> >> >> read loop and does not signal the interrupt. It seems it also leaves
>>> >> >> some data in a FIFO somewhere so next command on the flash returns
>>> >> >> garbage and fails.
>>> >> >
>>> >> > I observed something similar on MXS (mx28) SPI block. Do you use mixed
>>> >> > PIO/DMA mode perhaps ?
>>> >>
>>> >> The SPI driver uses PIO for short transfers and DMA for transfers
>>> >> longer than the controller FIFO. This seems to be the standard way to
>>> >> do things.It works flawlessly so long as submitting overly long DMA
>>> >> programs is avoided.
>>> >
>>> > Can you try doing JUST DMA, no PIO ? I remember seeing some bus
>>> > synchronisation issues when I did mixed PIO/DMA on the MXS and it was
>>> > nasty to track down. Just give pure DMA a go to see if the thing
>>> > stabilizes somehow.
>>>
>>> It's probably slower to set up DMA for 2-byte commands but it might
>>> work nonetheless.
>>
>> It is, the overhead will be considerable. It might help the stability
>> though. I'm really looking forward to the results!
>>
>
> Hello,
>
> this does not quite work.
>
> My test with spidev:
>
> # ./spinor /dev/spidev1.0
> Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
> Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
> Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
>
> I receive correct ID but spi-nor complains it does not know ID 00 c8 60.
> IIRC garbage should be sent only at the time command is transferred so
> only one byte of garbage should be received. Also the garbage tends to
> be the last state of the data output - all 0 or all 1.
> So it seems using DMA for all transfers including 1-byte commands
> results in (some?) received data getting an extra 00 prefix.
>

> I also managed to lock up the controller completely since there is
> some error passing the SPI speed somewhere :(
>
> [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
> [ 1352.977540] spidev spi1.0: spi mode 0
> [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
> [ 1352.977582] spidev spi1.0: msb first
> [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
> [ 1352.977620] spidev spi1.0: 0 bits per word
> [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max --> 0
> [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
> src_clk sclk_spi1 mode bpw 8
> [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
> bpw 8 speed -1604378624
> [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
> src_clk sclk_spi1 mode bpw 8
> [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
> [ 1352.977797] dma-pl330 121b0000.pdma: setting up request on thread 1

Hmm, on a second thought it probably works as expected more or less.

The nonsensical value was passed from application and there is no
guard against that.

Since I don't do PIO the controller remains locked up indefinitely.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1191628

On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:

Hi!

[...]

> >>> It's probably slower to set up DMA for 2-byte commands but it might
> >>> work nonetheless.
> >> 
> >> It is, the overhead will be considerable. It might help the stability
> >> though. I'm really looking forward to the results!
> > 
> > Hello,
> > 
> > this does not quite work.
> > 
> > My test with spidev:
> > 
> > # ./spinor /dev/spidev1.0
> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
> > 
> > I receive correct ID but spi-nor complains it does not know ID 00 c8 60.
> > IIRC garbage should be sent only at the time command is transferred so
> > only one byte of garbage should be received. Also the garbage tends to
> > be the last state of the data output - all 0 or all 1.
> > So it seems using DMA for all transfers including 1-byte commands
> > results in (some?) received data getting an extra 00 prefix.
> > 
> > 
> > I also managed to lock up the controller completely since there is
> > some error passing the SPI speed somewhere :(
> > 
> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
> > 0 [ 1352.977540] spidev spi1.0: spi mode 0
> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
> > 0 [ 1352.977582] spidev spi1.0: msb first
> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
> > 0 [ 1352.977620] spidev spi1.0: 0 bits per word
> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max
> > --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
> > src_clk sclk_spi1 mode bpw 8
> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
> > bpw 8 speed -1604378624
> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
> > src_clk sclk_spi1 mode bpw 8
> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using
> > dma [ 1352.977797] dma-pl330 121b0000.pdma: setting up request on thread
> > 1
> 
> Hmm, on a second thought it probably works as expected more or less.
> 
> The nonsensical value was passed from application and there is no
> guard against that.
> 
> Since I don't do PIO the controller remains locked up indefinitely.

I have to admit, I don't quite understand the above. I also don't quite know
what your spidev test does. Can you possibly just bind a regular SPI NOR driver 
and run mtdtests to see if it is stable ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1191735

On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
> On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
>
> Hi!
>
> [...]
>
>> >>> It's probably slower to set up DMA for 2-byte commands but it might
>> >>> work nonetheless.
>> >>
>> >> It is, the overhead will be considerable. It might help the stability
>> >> though. I'm really looking forward to the results!
>> >
>> > Hello,
>> >
>> > this does not quite work.
>> >
>> > My test with spidev:
>> >
>> > # ./spinor /dev/spidev1.0
>> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
>> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
>> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
>> >
>> > I receive correct ID but spi-nor complains it does not know ID 00 c8 60.
>> > IIRC garbage should be sent only at the time command is transferred so
>> > only one byte of garbage should be received. Also the garbage tends to
>> > be the last state of the data output - all 0 or all 1.
>> > So it seems using DMA for all transfers including 1-byte commands
>> > results in (some?) received data getting an extra 00 prefix.
>> >
>> >
>> > I also managed to lock up the controller completely since there is
>> > some error passing the SPI speed somewhere :(
>> >
>> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977540] spidev spi1.0: spi mode 0
>> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977582] spidev spi1.0: msb first
>> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977620] spidev spi1.0: 0 bits per word
>> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max
>> > --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
>> > src_clk sclk_spi1 mode bpw 8
>> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
>> > bpw 8 speed -1604378624
>> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
>> > src_clk sclk_spi1 mode bpw 8
>> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using
>> > dma [ 1352.977797] dma-pl330 121b0000.pdma: setting up request on thread
>> > 1
>>
>> Hmm, on a second thought it probably works as expected more or less.
>>
>> The nonsensical value was passed from application and there is no
>> guard against that.
>>
>> Since I don't do PIO the controller remains locked up indefinitely.
>
> I have to admit, I don't quite understand the above. I also don't quite know
> what your spidev test does.

It does a full duplex transfer sending what is printed and printing
what is received.

> Can you possibly just bind a regular SPI NOR driver
> and run mtdtests to see if it is stable ?

I can if I use PIO for short transfers. Using DMA for all transfers
results in the received data prefixed with 00 so the NOR flash
identification fails. Admittedly I have no idea what the flash memory
actually contains so if all DMA reads were always prefixed with 00 I
could not tell. I vaguely recall reading the whole content and parsing
the

I can probably make the minimum length for DMA configurable so I can
fall back to PIO when the controler locks up. It seems setting up a
PIO transfer makes it work again.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1192890

On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
> On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
>
> Hi!
>
> [...]
>
>> >>> It's probably slower to set up DMA for 2-byte commands but it might
>> >>> work nonetheless.
>> >>
>> >> It is, the overhead will be considerable. It might help the stability
>> >> though. I'm really looking forward to the results!
>> >
>> > Hello,
>> >
>> > this does not quite work.
>> >
>> > My test with spidev:
>> >
>> > # ./spinor /dev/spidev1.0
>> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
>> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
>> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
>> >
>> > I receive correct ID but spi-nor complains it does not know ID 00 c8 60.
>> > IIRC garbage should be sent only at the time command is transferred so
>> > only one byte of garbage should be received. Also the garbage tends to
>> > be the last state of the data output - all 0 or all 1.
>> > So it seems using DMA for all transfers including 1-byte commands
>> > results in (some?) received data getting an extra 00 prefix.
>> >
>> >
>> > I also managed to lock up the controller completely since there is
>> > some error passing the SPI speed somewhere :(
>> >
>> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977540] spidev spi1.0: spi mode 0
>> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977582] spidev spi1.0: msb first
>> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max -->
>> > 0 [ 1352.977620] spidev spi1.0: 0 bits per word
>> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz max
>> > --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
>> > src_clk sclk_spi1 mode bpw 8
>> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
>> > bpw 8 speed -1604378624
>> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
>> > src_clk sclk_spi1 mode bpw 8
>> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using
>> > dma [ 1352.977797] dma-pl330 121b0000.pdma: setting up request on thread
>> > 1
>>
>> Hmm, on a second thought it probably works as expected more or less.
>>
>> The nonsensical value was passed from application and there is no
>> guard against that.
>>
>> Since I don't do PIO the controller remains locked up indefinitely.
>
> I have to admit, I don't quite understand the above. I also don't quite know
> what your spidev test does. Can you possibly just bind a regular SPI NOR driver
> and run mtdtests to see if it is stable ?

Ok, so here is some summary.

I have a NOR flash attached to a s3c64xx SPI controller with 64byte
fifo and a pl330 dma controller.

Normally DMA controller is used for transfers that do not fit into the FIFO.

I tried adding the flash memory ID to the spi-nor driver table and
adding a DT node for it.

The flash is rated at 120MHz so I used that speed but the ID was
bit-shifted and identification failed. There is DT property
samsung,spi-feedback-delay for addressing this and at 120MHz it must
be 2 or 3 on this board. 40MHz works with default value 0.

The next step after identification worked was to try reading the flash
content. For this the DMA controller is used because data is
transferred in blocks larger than 64 bytes. When reading the whole 4MB
flash the transfer failed silently. I got a 4MB file of all ones or
all zeroes.

It turns out that

 - the pl330 locks up when transfering large amount of data.
Specifically, the maximum power of 2 sized transfer at 120MHz is 128
bytes and 64k at 40MHz. Transferring more than this results in the
pl330 locking up and never signalling completion of the transfer. Data
is left in FIFO which causes subsequent commands to fail since garbage
is returned instead of command reply. Also subsequent read may cause
I/O error or or return an empty page depending on the garbage around.

- the I/O errors are not checked in spi-nor at all which leads to
silent data corruption.

On a suggestion that this may improve reliability I changed the
s3c64xx driver to use DMA for all transfers. This caused
identification to fail in spin-nor because the ID was prefixed with
extra 00  byte. Testing with spidev confirmed that everything is
prefixed with extra 00. Also when the DMA controller locked up no
transfers were possible anymore. When DMA was not used for sending
commands the controller would recover on next command. I made the
option to always use DMA configurable and it turns out that the
returned data is prefixed with 00 only when no transfer without DMA
was ever made. Loading the spi-nor driver with the dma-only option off
and then with dma-only option on results in correct operation. Only
loading the dma-only driver first causes the 00 prefix.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web