Message-ID: <CAOMqctTQ906nu4_VjXWDRZ23uRbxEHMTKs6TpYy+g9EBAd1M7Q@mail.gmail.com>
Date:	Thu, 30 Jul 2015 14:18:09 +0200
From:	Michal Suchanek <hramrach@...il.com>
To:	Marek Vasut <marex@...x.de>
Cc:	Vinod Koul <vinod.koul@...el.com>,
	MTD Maling List <linux-mtd@...ts.infradead.org>,
	"linux-samsung-soc@...r.kernel.org" 
	<linux-samsung-soc@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	linux-spi <linux-spi@...r.kernel.org>,
	dmaengine <dmaengine@...r.kernel.org>
Subject: Re: [PATCH 08/11] MTD: m25p80: Add option to limit SPI transfer size.

On 30 July 2015 at 13:24, Marek Vasut <marex@...x.de> wrote:
> On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
>> On 27 July 2015 at 19:43, Marek Vasut <marex@...x.de> wrote:
>> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
>> >> On 24 July 2015 at 10:34, Marek Vasut <marex@...x.de> wrote:
>> >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
>> >> 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.
>> >
>> > Hypothesis:
>> > I think this might just be that the controller didn't catch all the
>> > inbound clock ticks and thus counted less inbound data than it was
>> > set up to receive. The controller thus waits for more data.
>>
>> The flash chip can transfer data as long as you keep the clock going,
>> especially when you transfer 64k off a 4M flash.
>>
>> The read command is bound just by stopping the clock. There is always
>> more data to be had.
>
> Sure, if you keep clocking the chip, the data will keep going. Is the
> chip being clocked ?

There is always data. Like in whenever you want to read SPI data you
sample the input pin and watever you sample is data so far as SPI bus
is concerned.

>
> Doesn't the PL330 have some kind of a counter register where you can check
> how much data were received so far ? That should be sufficient to verify
> this hypothesis of mine.

Could check that, yes. Maybe I could read the counter in a function
which dmaengine client uses to tear down the dma transfer.

On a related note I enabled more prints on the spidev test program.

I have code that tests maximum transfer size up to the 4k spidev limit
and it can transfer full 4k buffer of NOPs at 80MHz.

The recieved data is all ones except a 00 at the start. So it looks
like read-only transfers fail but full-duplex sort of work. And it
reproduces the 00 prefix that was seen in the dma-only setup in
otherwise working setup :-S

An attempt to read a page of data using the fast-read command fails
with timeout.

>
>> >> 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.
>> >
>> > So the driver for the DMA controller might need to be augmented to handle
>> > this case -- add a timeout and in case DMA times out, drain the FIFO to
>> > make sure subsequent transfers do not fail.
>>
>> There is no way to add timeout in the DMA driver since it does not
>> know the SPI clock.
>>
>> The timeout is handled by the SPI driver and it should be possible to
>> augment it to drain the FIFO when DMA fails so long as FIFO level is
>> readable somewhere.
>
> If the DMA doesn't complete in certain amount of time, it should time out
> I'd say. Don't you think ?

No. The DMA  driver has no idea if the transfer is at 133MHz, 40MHz,
1MHz, 1kHz, whatever.

On the other hand, adding a flush_fifo in the SPI driver after DMA
failure allows probing the chip reliably by realoding the driver even
just after a failed transfer.

>
>> >> - 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.
>> >
>> > The determinism of this is in fact excellent news.
>> >
>> >> 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.
>> >
>> > Can we conclude that the PIO codepath somehow programs a register
>> > somewhere which gets rid of this 0x00 prefix ? If so, this should then
>> > also be part of the DMA codepath, or even better, this should be set in
>> > probe() somewhere.
>>
>> Yes, it looks like it.
>
> Did you find what this could be please ?

The suspicious function is enable_datapath in the s3c64xx driver.
There is even a comment about keeping the clock going on half-duplex
transfers.

I did not get to dissecting it so far.

Thanks

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

Powered by blists - more mailing lists