On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek <[email protected]> wrote:
> On sunxi the SPI controller currently does not have DMA support and fails
> any transfer larger than 63 bytes.
This is a driver limitation, not a hardware limitation.
> On Exynos the pl330 DMA controller fails any transfer larger than 64kb
> when using slower speed like 40MHz and any transfer larger than 128bytes
> when running at 133MHz.
This may be a driver bug.
> The best thing is that in both cases the controller can just lock up and
> never finish potentially leaving the hardware in unusable state.
>
> So it is required that the m25p80 driver actively prevents doing
> transfers that are too large for the current driver state on a
> particular piece of hardware.
OK.
> Signed-off-by: Michal Suchanek <[email protected]>
>
> --
>
> Update commit message and documentation text.
> ---
> .../devicetree/bindings/mtd/jedec,spi-nor.txt | 6 ++++++
> drivers/mtd/devices/m25p80.c | 24 ++++++++++++++++++++--
> 2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> index 2bee681..4e63ae8 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
> @@ -19,6 +19,12 @@ Optional properties:
> all chips and support for it can not be detected at runtime.
> Refer to your chips' datasheet to check if this is supported
> by your chip.
> +- linux,max_tx_len : With some SPI controller drivers possible transfer size is
> + limited. This may be hardware or driver bug.
> + Transfer data in chunks no larger than this value.
> + Using this option may severely degrade performance and
> + possibly flash memory life when max_tx_len is smaller than
> + flash page size (typically 256 bytes)
DT describes the hardware, not buggy drivers.
So IMHO this doesn't belong in DT, but it can be a field in struct spi_master.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 4 June 2015 at 08:42, Geert Uytterhoeven <[email protected]> wrote:
> On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek <[email protected]> wrote:
>> On sunxi the SPI controller currently does not have DMA support and fails
>> any transfer larger than 63 bytes.
>
> This is a driver limitation, not a hardware limitation.
>
>> On Exynos the pl330 DMA controller fails any transfer larger than 64kb
>> when using slower speed like 40MHz and any transfer larger than 128bytes
>> when running at 133MHz.
>
> This may be a driver bug.
>
>> The best thing is that in both cases the controller can just lock up and
>> never finish potentially leaving the hardware in unusable state.
>>
>> So it is required that the m25p80 driver actively prevents doing
>> transfers that are too large for the current driver state on a
>> particular piece of hardware.
>
> OK.
>
> DT describes the hardware, not buggy drivers.
>
> So IMHO this doesn't belong in DT, but it can be a field in struct spi_master.
>
Unfortunately, this cannot be a field in struct spi_master. The value
can and does vary with device configuration and device configuration
is only available in DT.
For example, on the Snow board one working configuration is
40MHz spi bus speed, feedback delay 0 and maximum transfer size 64k.
Another working configuration is 133MHz bus speed, feedback delay 3,
and maximum transfer size 128 bytes.
You might want to try to run the bus at 60MHz or 80MHz and then the
values would probably again be different.
The first two values are set in DT so the logical place for setting
the third is also in DT.
Otherwise you would need some in-kernel table of these settings.
Thanks
Michal
On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote:
> You might want to try to run the bus at 60MHz or 80MHz and then the
> values would probably again be different.
>
> The first two values are set in DT so the logical place for setting
> the third is also in DT.
>
> Otherwise you would need some in-kernel table of these settings.
Or a formula.
Thanks,
Richard
On 4 June 2015 at 19:15, Richard Cochran <[email protected]> wrote:
> On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote:
>> You might want to try to run the bus at 60MHz or 80MHz and then the
>> values would probably again be different.
>>
>> The first two values are set in DT so the logical place for setting
>> the third is also in DT.
>>
>> Otherwise you would need some in-kernel table of these settings.
>
> Or a formula.
>
This formula probably needs to take into account
- the unknown reason for the pl330 to fail transfer
- the device transfer speed and transfer phase as set in DT
- possibly device-specific latency and board-specific trace design
and assembly tolerances
Seriously, until I have at least a vague idea why the transfer fails I
am not comfortable pulling some formula out of thin air and pretending
I have a working patch.
On the other hand, a parameter you can set in the DT and which comes
with a suggested value which can be tuned depending on the system
seems more viable.
Thanks
Michal
On Wednesday, July 15, 2015 at 11:45:07 AM, Michal Suchanek wrote:
> On 4 June 2015 at 19:15, Richard Cochran <[email protected]> wrote:
> > On Thu, Jun 04, 2015 at 10:31:45AM +0200, Michal Suchanek wrote:
> >> You might want to try to run the bus at 60MHz or 80MHz and then the
> >> values would probably again be different.
> >>
> >> The first two values are set in DT so the logical place for setting
> >> the third is also in DT.
> >>
> >> Otherwise you would need some in-kernel table of these settings.
> >
> > Or a formula.
>
> This formula probably needs to take into account
>
> - the unknown reason for the pl330 to fail transfer
Shouldn't that be fixed at the PL330 level ? This looks like fixing
a problem at the wrong place :)
> - the device transfer speed and transfer phase as set in DT
> - possibly device-specific latency and board-specific trace design
> and assembly tolerances
If the design is broken, then cap the speed as for normal SPI device.
> Seriously, until I have at least a vague idea why the transfer fails I
> am not comfortable pulling some formula out of thin air and pretending
> I have a working patch.
>
> On the other hand, a parameter you can set in the DT and which comes
> with a suggested value which can be tuned depending on the system
> seems more viable.
The problem is, if you add a new DT binding, you'd have to support it
forever, no matter how bad idea that binding turned out to be.
Best regards,
Marek Vasut
Hi Michal,
On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
> The problem is, if you add a new DT binding, you'd have to support it
> forever, no matter how bad idea that binding turned out to be.
Agreed, and a solid NAK to this patch. I could have sworn I gave such a
response when this was originally being discussed a month ago.
AFAICT, you have one of two general approaches available to you:
1. Fix up the SPI driver so that it knows how to break large SPI
transfers up into smaller segments that its constituent hardware (DMA
controllers, fast clocks, etc.) can handle.
2. Utilize/create a parameter within the SPI subsystem to communicate
that the SPI master has a limited max transfer size (notably: NOT a
per-device DT property, but a SPI API property), and modify SPI device
drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I
thought he suggested it somewhere.
It is most definitely wrong to put this information solely in the slave
device DT (i.e., for the flash), when it is not a property of the flash
device at all. It's a property of the SPI master and/or its clocks and
DMA controllers.
Brian
On Wednesday, July 15, 2015 at 05:59:46 PM, Brian Norris wrote:
> Hi Michal,
Hi all,
> On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
> > The problem is, if you add a new DT binding, you'd have to support it
> > forever, no matter how bad idea that binding turned out to be.
>
> Agreed, and a solid NAK to this patch. I could have sworn I gave such a
> response when this was originally being discussed a month ago.
>
> AFAICT, you have one of two general approaches available to you:
>
> 1. Fix up the SPI driver so that it knows how to break large SPI
> transfers up into smaller segments that its constituent hardware (DMA
> controllers, fast clocks, etc.) can handle.
I think this might actually be easier -- just do a transfer where you
don't toggle CS and just stops the clock at the last bit, then do another
(multiple) transfers which don't toggle CS at all, then finally do a
transfer which toggles a CS at the end. This should be pretty trivial
to do and I think for example spi-mxs.c does this.
Best regards,
Marek Vasut
On Wed, Jul 15, 2015 at 07:15:50PM +0200, Marek Vasut wrote:
> On Wednesday, July 15, 2015 at 05:59:46 PM, Brian Norris wrote:
> > 1. Fix up the SPI driver so that it knows how to break large SPI
> > transfers up into smaller segments that its constituent hardware (DMA
> > controllers, fast clocks, etc.) can handle.
BTW, Mark Brown already commented on this approach:
http://lists.infradead.org/pipermail/linux-mtd/2015-May/059364.html
I quote:
| With modern drivers using transfer_one() where we have control of the
| chip select we do also have the ability to add support to the core for
| chopping large transfers up into smaller ones that the hardware can
| handle transparently. That won't for everything though since it depends
| on us being able to manually control the chip select which not all
| hardware can do.
> I think this might actually be easier -- just do a transfer where you
> don't toggle CS and just stops the clock at the last bit, then do another
> (multiple) transfers which don't toggle CS at all, then finally do a
> transfer which toggles a CS at the end.
Sounds OK to me. And as Mark noted, this could probably be done in the
core. I suppose Mark could suggest whether the most expedient path is to
hack the buggy driver to do this, or whether reworking the SPI core to
have the appropriate spi_master field(s) (and caveats) to support this.
> This should be pretty trivial
> to do and I think for example spi-mxs.c does this.
I don't think spi-mxs.c really does this; it chooses between PIO and DMA
based on length, but with either option, it runs through each SPI
transfer in one go. You might be confused by the fact that this driver
implements ->transfer_one_message instead of ->transfer_one, so it has
to loop through all transfers in the message.
(IIUC, spi-mxs.c could easily be rewritten to use ->transfer, and some
of that not-too-complicated boilerplate could be killed off.)
Brian
On Thursday, July 16, 2015 at 03:19:35 AM, Brian Norris wrote:
> On Wed, Jul 15, 2015 at 07:15:50PM +0200, Marek Vasut wrote:
> > On Wednesday, July 15, 2015 at 05:59:46 PM, Brian Norris wrote:
> > > 1. Fix up the SPI driver so that it knows how to break large SPI
> > > transfers up into smaller segments that its constituent hardware (DMA
> > > controllers, fast clocks, etc.) can handle.
>
> BTW, Mark Brown already commented on this approach:
>
> http://lists.infradead.org/pipermail/linux-mtd/2015-May/059364.html
>
> I quote:
> | With modern drivers using transfer_one() where we have control of the
> | chip select we do also have the ability to add support to the core for
> | chopping large transfers up into smaller ones that the hardware can
> | handle transparently. That won't for everything though since it depends
> | on us being able to manually control the chip select which not all
> | hardware can do.
> |
> > I think this might actually be easier -- just do a transfer where you
> > don't toggle CS and just stops the clock at the last bit, then do another
> > (multiple) transfers which don't toggle CS at all, then finally do a
> > transfer which toggles a CS at the end.
>
> Sounds OK to me. And as Mark noted, this could probably be done in the
> core. I suppose Mark could suggest whether the most expedient path is to
> hack the buggy driver to do this, or whether reworking the SPI core to
> have the appropriate spi_master field(s) (and caveats) to support this.
Yep, agreed.
> > This should be pretty trivial
> > to do and I think for example spi-mxs.c does this.
>
> I don't think spi-mxs.c really does this; it chooses between PIO and DMA
> based on length, but with either option, it runs through each SPI
> transfer in one go. You might be confused by the fact that this driver
> implements ->transfer_one_message instead of ->transfer_one, so it has
> to loop through all transfers in the message.
It does chop up the DMA transfers between multiple runs of the DMA engine
IIRC. But it does so within the driver, which apparently is not the way
to go :)
> (IIUC, spi-mxs.c could easily be rewritten to use ->transfer, and some
> of that not-too-complicated boilerplate could be killed off.)
Most likely, yes.
Best regards,
Marek Vasut
Hello,
On 15 July 2015 at 17:59, Brian Norris <[email protected]> wrote:
> Hi Michal,
>
> On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
>> The problem is, if you add a new DT binding, you'd have to support it
>> forever, no matter how bad idea that binding turned out to be.
>
> Agreed, and a solid NAK to this patch. I could have sworn I gave such a
> response when this was originally being discussed a month ago.
>
> AFAICT, you have one of two general approaches available to you:
>
> 1. Fix up the SPI driver so that it knows how to break large SPI
> transfers up into smaller segments that its constituent hardware (DMA
> controllers, fast clocks, etc.) can handle.
>
> 2. Utilize/create a parameter within the SPI subsystem to communicate
> that the SPI master has a limited max transfer size (notably: NOT a
> per-device DT property, but a SPI API property), and modify SPI device
> drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I
> thought he suggested it somewhere.
It is not known what exactly is limited here.
It seems that the pl330 fails but it is not possible to transfer that
much data over the spi bus in one go without the help of the pl330.
With either approach the limit depends on the SPI transfer settings
which are known the the SPI driver. The pl330 driver is oblivious to
these because it just transfers data from one port to another port and
has no idea that the port is wired to SPI in the SoC.
On the other hand, AFAICT the SPI driver only allocates a DMA channel
which it receives through DT binding and is oblivious to the fact the
DMA channel lives on a pl330. It could probably determine that the
channel is indeed driven by a pl330. I don't think it's a great idea
to add device-specific handling to a generic dmaengine driver or a
dmaengine-spiecific handling to a SPI driver.
It's technically possible to pass SPI transfer parameters to the
dmaengine driver prior to transfer and the dmaengine could impose some
limitation based on those parameters. However, generalising this to
drivers other than SPI might be problematic. Should this interface
also handle i2c parameters, VE parameters, audio parameters, ethernet
parameters, etc?
In the DT you have the information that this particular device is
connected to a Samsung SPI controller connected to a pl330 dma engine.
>
> It is most definitely wrong to put this information solely in the slave
> device DT (i.e., for the flash), when it is not a property of the flash
> device at all. It's a property of the SPI master and/or its clocks and
> DMA controllers.
However, the clocks are set by the parameters of the device, not the
parameters of the master. So the limitation applies for the master
with the settings of the particular slave device. Different slave
settings do impose different master limitations.
Thanks
Michal
On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote:
> Hello,
>
> On 15 July 2015 at 17:59, Brian Norris <[email protected]> wrote:
> > Hi Michal,
> >
> > On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
> >> The problem is, if you add a new DT binding, you'd have to support it
> >> forever, no matter how bad idea that binding turned out to be.
> >
> > Agreed, and a solid NAK to this patch. I could have sworn I gave such a
> > response when this was originally being discussed a month ago.
> >
> > AFAICT, you have one of two general approaches available to you:
> >
> > 1. Fix up the SPI driver so that it knows how to break large SPI
> > transfers up into smaller segments that its constituent hardware (DMA
> > controllers, fast clocks, etc.) can handle.
> >
> > 2. Utilize/create a parameter within the SPI subsystem to communicate
> > that the SPI master has a limited max transfer size (notably: NOT a
> > per-device DT property, but a SPI API property), and modify SPI device
> > drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I
> > thought he suggested it somewhere.
>
> It is not known what exactly is limited here.
>
> It seems that the pl330 fails but it is not possible to transfer that
> much data over the spi bus in one go without the help of the pl330.
>
> With either approach the limit depends on the SPI transfer settings
> which are known the the SPI driver. The pl330 driver is oblivious to
> these because it just transfers data from one port to another port and
> has no idea that the port is wired to SPI in the SoC.
>
> On the other hand, AFAICT the SPI driver only allocates a DMA channel
> which it receives through DT binding and is oblivious to the fact the
> DMA channel lives on a pl330. It could probably determine that the
> channel is indeed driven by a pl330. I don't think it's a great idea
> to add device-specific handling to a generic dmaengine driver or a
> dmaengine-spiecific handling to a SPI driver.
>
> It's technically possible to pass SPI transfer parameters to the
> dmaengine driver prior to transfer and the dmaengine could impose some
> limitation based on those parameters. However, generalising this to
> drivers other than SPI might be problematic. Should this interface
> also handle i2c parameters, VE parameters, audio parameters, ethernet
> parameters, etc?
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...
Thanks
--
~Vinod
Hello,
On 21 July 2015 at 06:29, Vinod Koul <[email protected]> wrote:
> On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote:
>> Hello,
>>
>> On 15 July 2015 at 17:59, Brian Norris <[email protected]> wrote:
>> > Hi Michal,
>> >
>> > On Wed, Jul 15, 2015 at 01:52:27PM +0200, Marek Vasut wrote:
>> >> The problem is, if you add a new DT binding, you'd have to support it
>> >> forever, no matter how bad idea that binding turned out to be.
>> >
>> > Agreed, and a solid NAK to this patch. I could have sworn I gave such a
>> > response when this was originally being discussed a month ago.
>> >
>> > AFAICT, you have one of two general approaches available to you:
>> >
>> > 1. Fix up the SPI driver so that it knows how to break large SPI
>> > transfers up into smaller segments that its constituent hardware (DMA
>> > controllers, fast clocks, etc.) can handle.
>> >
>> > 2. Utilize/create a parameter within the SPI subsystem to communicate
>> > that the SPI master has a limited max transfer size (notably: NOT a
>> > per-device DT property, but a SPI API property), and modify SPI device
>> > drivers (like m25p80) to honor it. Mark Brown seemed open to this, and I
>> > thought he suggested it somewhere.
>>
>> It is not known what exactly is limited here.
>>
>> It seems that the pl330 fails but it is not possible to transfer that
>> much data over the spi bus in one go without the help of the pl330.
>>
>> With either approach the limit depends on the SPI transfer settings
>> which are known the the SPI driver. The pl330 driver is oblivious to
>> these because it just transfers data from one port to another port and
>> has no idea that the port is wired to SPI in the SoC.
>>
>> On the other hand, AFAICT the SPI driver only allocates a DMA channel
>> which it receives through DT binding and is oblivious to the fact the
>> DMA channel lives on a pl330. It could probably determine that the
>> channel is indeed driven by a pl330. I don't think it's a great idea
>> to add device-specific handling to a generic dmaengine driver or a
>> dmaengine-spiecific handling to a SPI driver.
>>
>> It's technically possible to pass SPI transfer parameters to the
>> dmaengine driver prior to transfer and the dmaengine could impose some
>> limitation based on those parameters. However, generalising this to
>> drivers other than SPI might be problematic. Should this interface
>> also handle i2c parameters, VE parameters, audio parameters, ethernet
>> parameters, etc?
> 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.
Thanks
Michal
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.
--
~Vinod
On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
However, if this was not tied to the particular slave setting picked
in the current DT a formula would be needed that translates arbitrary
client settings to transfer size limit and there would be need to
somehow get the client settings to the formula in the dmaengine
driver.
Thanks
Michal
On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
> However, if this was not tied to the particular slave setting picked
> in the current DT a formula would be needed that translates arbitrary
> client settings to transfer size limit and there would be need to
> somehow get the client settings to the formula in the dmaengine
> driver.
>
> Thanks
>
> Michal
Best regards,
Marek Vasut
On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
> On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
Thanks
Michal
On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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 ? Do you have the option to connect a bus analyzer?
I can probably offer you some tools, I'm in Prague ...
Best regards,
Marek Vasut
On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
> On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
>> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
> Do you have the option to connect a bus analyzer?
> I can probably offer you some tools, I'm in Prague ...
The flash chip is accessible when removing the bottom cover. It is
VSOP8 package slightly lower than SOP8 so attaching clips to it might
be a bit problematic. That's the only accessible part. Everything
other than SPI is inside the SoC.
Since SPI has no verification whatsoever the chip might be completely
dead and you can still read fine all zeroes or all ones when
attempting a read from it. I observed this behaviour when I used a
flash chip in a socket and it was not firmly seated. It was with a
different SPI controller, though.
Thanks
Michal
On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
> On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
> >> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> >> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
> > Do you have the option to connect a bus analyzer?
> > I can probably offer you some tools, I'm in Prague ...
>
> The flash chip is accessible when removing the bottom cover. It is
> VSOP8 package slightly lower than SOP8 so attaching clips to it might
> be a bit problematic. That's the only accessible part. Everything
> other than SPI is inside the SoC.
That might be doable, though you might want to try the above thing first.
> Since SPI has no verification whatsoever the chip might be completely
> dead and you can still read fine all zeroes or all ones when
> attempting a read from it. I observed this behaviour when I used a
> flash chip in a socket and it was not firmly seated. It was with a
> different SPI controller, though.
You should run into issues as soon as the SPI NOR framework tries to read
status register, no ?
On 22 July 2015 at 10:24, Marek Vasut <[email protected]> wrote:
> On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
>> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>> >> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
>> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> >> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
I will give it a go.
>
>> > Do you have the option to connect a bus analyzer?
>> > I can probably offer you some tools, I'm in Prague ...
>>
>> The flash chip is accessible when removing the bottom cover. It is
>> VSOP8 package slightly lower than SOP8 so attaching clips to it might
>> be a bit problematic. That's the only accessible part. Everything
>> other than SPI is inside the SoC.
>
> That might be doable, though you might want to try the above thing first.
>
>> Since SPI has no verification whatsoever the chip might be completely
>> dead and you can still read fine all zeroes or all ones when
>> attempting a read from it. I observed this behaviour when I used a
>> flash chip in a socket and it was not firmly seated. It was with a
>> different SPI controller, though.
>
> You should run into issues as soon as the SPI NOR framework tries to read
> status register, no ?
Yes, when the DMA transfer fails the next command fails due to garbage
lying around. However, you can unload the SPI NOR driver, load spidev
driver, and read enough garbage to empty the fifos. Then the flash
identifies as normal again and you can access it.
When the flash is not seated properly and acts autistic you get all
0xff or all 0 back whatever you send to it, obvously. The
identification by the SPI NOR driver fails then.
Thanks
Michal
On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote:
> On 22 July 2015 at 10:24, Marek Vasut <[email protected]> wrote:
> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
> >> On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
> >> >> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> >> >> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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!
> I will give it a go.
>
> >> > Do you have the option to connect a bus analyzer?
> >> > I can probably offer you some tools, I'm in Prague ...
> >>
> >> The flash chip is accessible when removing the bottom cover. It is
> >> VSOP8 package slightly lower than SOP8 so attaching clips to it might
> >> be a bit problematic. That's the only accessible part. Everything
> >> other than SPI is inside the SoC.
> >
> > That might be doable, though you might want to try the above thing first.
> >
> >> Since SPI has no verification whatsoever the chip might be completely
> >> dead and you can still read fine all zeroes or all ones when
> >> attempting a read from it. I observed this behaviour when I used a
> >> flash chip in a socket and it was not firmly seated. It was with a
> >> different SPI controller, though.
> >
> > You should run into issues as soon as the SPI NOR framework tries to read
> > status register, no ?
>
> Yes, when the DMA transfer fails the next command fails due to garbage
> lying around. However, you can unload the SPI NOR driver, load spidev
> driver, and read enough garbage to empty the fifos. Then the flash
> identifies as normal again and you can access it.
Yikes :(
> When the flash is not seated properly and acts autistic you get all
> 0xff or all 0 back whatever you send to it, obvously. The
> identification by the SPI NOR driver fails then.
>
> Thanks
>
> Michal
On 22 July 2015 at 11:01, Marek Vasut <[email protected]> wrote:
> On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 10:24, Marek Vasut <[email protected]> wrote:
>> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
>> >> On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
>> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>> >> >> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
>> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> >> >> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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.
[ 26.702690] spi spi1.0: setup mode 0, 8 bits/w, 40000000 Hz max --> 0
[ 26.703474] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 26.703534] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
bpw 8 speed 40000000
[ 26.703543] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
[ 26.703568] dma-pl330 121b0000.pdma: setting up request on thread 1
[ 26.703576] bc041200: DMAMOV CCR 0x800201
[ 26.703585] bc041206: DMAMOV SAR 0x6e151280
[ 26.703593] bc04120c: DMAMOV DAR 0x12d30018
[ 26.703601] bc041212: DMAWFPS 5
[ 26.703609] bc041214: DMALDA
[ 26.703615] bc041215: DMASTPS 5
[ 26.703625] bc041217: DMAFLUSHP 5
[ 26.703636] bc041219: DMASEV 1
[ 26.703645] bc04121b: DMAEND
[ 26.703668] dma-pl330 121b0000.pdma: event signalled on thread id 1
[ 26.703690] spi_master spi1: wait_for_dma: waiting for 100ms
transferring 1bytes@40000000Hz
[ 26.703699] spi_master spi1: wait_for_dma: waited 0 ms
[ 26.703711] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
bpw 8 speed 40000000
[ 26.703718] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
[ 26.703728] dma-pl330 121b0000.pdma: setting up request on thread 0
[ 26.703733] bc041000: DMAMOV CCR 0x804200
[ 26.703742] bc041006: DMAMOV SAR 0x12d3001c
[ 26.703749] bc04100c: DMAMOV DAR 0x6e151281
[ 26.703755] bc041012: DMALP_1 5
[ 26.703763] bc041014: DMAWFPS 4
[ 26.703769] bc041016: DMALDPS 4
[ 26.703775] bc041018: DMASTA
[ 26.703781] bc041019: DMAFLUSHP 4
[ 26.703787] bc04101b: DMALPENDA_1 bjmpto_7
[ 26.703795] bc04101d: DMASEV 0
[ 26.703801] bc04101f: DMAEND
[ 26.703815] spi_master spi1: wait_for_dma: waiting for 100ms
transferring 6bytes@40000000Hz
[ 26.703820] dma-pl330 121b0000.pdma: event signalled on thread id 0
[ 26.703837] spi_master spi1: wait_for_dma: waited 0 ms
[ 26.703866] m25p80 spi1.0: unrecognized JEDEC id bytes: 00, c8, 60
[ 26.703885] m25p80: probe of spi1.0 failed with error -2
[ 26.703905] s3c64xx-spi 12d30000.spi: registered child spi1.0
[ 65.079159] spi spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
[ 65.079343] spidev spi1.0: buggy DT: spidev listed directly in DT
[ 65.079350] ------------[ cut here ]------------
[ 65.079362] WARNING: CPU: 1 PID: 3951 at
/scratch/build/linux/drivers/spi/spidev.c:717
spidev_probe+0x1d4/0x1f0()
[ 65.079367] Modules linked in: ipv6
[ 65.079383] CPU: 1 PID: 3951 Comm: cat Not tainted
4.2.0-rc3-00167-ged73574 #43
[ 65.079389] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[ 65.079406] [<c00168bc>] (unwind_backtrace) from [<c0012fc4>]
(show_stack+0x10/0x14)
[ 65.079417] [<c0012fc4>] (show_stack) from [<c066fea8>]
(dump_stack+0x84/0xc4)
[ 65.079428] [<c066fea8>] (dump_stack) from [<c00299a0>]
(warn_slowpath_common+0x84/0xb4)
[ 65.079437] [<c00299a0>] (warn_slowpath_common) from [<c0029a6c>]
(warn_slowpath_null+0x1c/0x24)
[ 65.079446] [<c0029a6c>] (warn_slowpath_null) from [<c03f38a4>]
(spidev_probe+0x1d4/0x1f0)
[ 65.079457] [<c03f38a4>] (spidev_probe) from [<c03f1258>]
(spi_drv_probe+0x50/0x74)
[ 65.079468] [<c03f1258>] (spi_drv_probe) from [<c03a885c>]
(driver_probe_device+0x1fc/0x43c)
[ 65.079480] [<c03a885c>] (driver_probe_device) from [<c03a6a94>]
(bus_for_each_drv+0x64/0x98)
[ 65.079489] [<c03a6a94>] (bus_for_each_drv) from [<c03a8588>]
(__device_attach+0x8c/0x108)
[ 65.079498] [<c03a8588>] (__device_attach) from [<c03a7b08>]
(bus_probe_device+0x84/0x8c)
[ 65.079506] [<c03a7b08>] (bus_probe_device) from [<c03a5cb0>]
(device_add+0x414/0x5a4)
[ 65.079514] [<c03a5cb0>] (device_add) from [<c03f187c>]
(spi_add_device+0x94/0x16c)
[ 65.079524] [<c03f187c>] (spi_add_device) from [<c03f1d08>]
(of_register_spi_device+0x234/0x2fc)
[ 65.079532] [<c03f1d08>] (of_register_spi_device) from [<c03f3350>]
(of_spi_notify+0x88/0xd0)
[ 65.079542] [<c03f3350>] (of_spi_notify) from [<c0043170>]
(notifier_call_chain+0x44/0x84)
[ 65.079552] [<c0043170>] (notifier_call_chain) from [<c00434cc>]
(__blocking_notifier_call_chain+0x48/0x60)
[ 65.079561] [<c00434cc>] (__blocking_notifier_call_chain) from
[<c00434fc>] (blocking_notifier_call_chain+0x18/0x20)
[ 65.079572] [<c00434fc>] (blocking_notifier_call_chain) from
[<c0508350>] (__of_changeset_entry_notify+0x84/0xe0)
[ 65.079581] [<c0508350>] (__of_changeset_entry_notify) from
[<c0508c08>] (of_changeset_apply+0x78/0x140)
[ 65.079591] [<c0508c08>] (of_changeset_apply) from [<c050d5c8>]
(of_overlay_create+0x300/0x490)
[ 65.079601] [<c050d5c8>] (of_overlay_create) from [<c050d790>]
(of_overlay_create_store+0x38/0x50)
[ 65.079611] [<c050d790>] (of_overlay_create_store) from
[<c01375d4>] (kernfs_fop_write+0xb8/0x19c)
[ 65.079622] [<c01375d4>] (kernfs_fop_write) from [<c00daae8>]
(__vfs_write+0x20/0xd8)
[ 65.079631] [<c00daae8>] (__vfs_write) from [<c00db308>]
(vfs_write+0x90/0x164)
[ 65.079639] [<c00db308>] (vfs_write) from [<c00dbb30>] (SyS_write+0x44/0x9c)
[ 65.079647] [<c00dbb30>] (SyS_write) from [<c0010100>]
(ret_fast_syscall+0x0/0x3c)
[ 65.079654] ---[ end trace 4cf0fad6e5c33d55 ]---
[ 65.079957] s3c64xx-spi 12d30000.spi: registered child spi1.0
[ 91.215230] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
[ 91.215242] spidev spi1.0: spi mode 0
[ 91.215276] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
[ 91.215282] spidev spi1.0: msb first
[ 91.215314] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max --> 0
[ 91.215319] spidev spi1.0: 0 bits per word
[ 91.215351] spidev spi1.0: setup mode 0, 8 bits/w, 1000000 Hz max --> 0
[ 91.215424] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.215460] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
bpw 8 speed 1000000
[ 91.215467] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.215500] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
[ 91.215515] dma-pl330 121b0000.pdma: setting up request on thread 1
[ 91.215520] bc041200: DMAMOV CCR 0x800201
[ 91.215526] bc041206: DMAMOV SAR 0x6d842000
[ 91.215532] bc04120c: DMAMOV DAR 0x12d30018
[ 91.215538] bc041212: DMAWFPS 5
[ 91.215543] bc041214: DMALDA
[ 91.215548] bc041215: DMASTPS 5
[ 91.215553] bc041217: DMAFLUSHP 5
[ 91.215558] bc041219: DMASEV 1
[ 91.215563] bc04121b: DMAEND
[ 91.215577] dma-pl330 121b0000.pdma: setting up request on thread 0
[ 91.215581] bc041000: DMAMOV CCR 0x804200
[ 91.215586] bc041006: DMAMOV SAR 0x12d3001c
[ 91.215592] bc04100c: DMAMOV DAR 0x6d845000
[ 91.215597] bc041012: DMAWFPS 4
[ 91.215602] bc041014: DMALDPS 4
[ 91.215607] bc041016: DMASTA
[ 91.215612] bc041017: DMAFLUSHP 4
[ 91.215617] bc041019: DMASEV 0
[ 91.215622] bc04101b: DMAEND
[ 91.215632] dma-pl330 121b0000.pdma: event signalled on thread id 1
[ 91.215646] spi_master spi1: wait_for_dma: waiting for 100ms
transferring 1bytes@1000000Hz
[ 91.215650] dma-pl330 121b0000.pdma: event signalled on thread id 0
[ 91.215680] spi_master spi1: wait_for_dma: waited 0 ms
[ 91.215803] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.215836] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
bpw 8 speed 1000000
[ 91.215844] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.215878] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
[ 91.215887] dma-pl330 121b0000.pdma: setting up request on thread
1[ 91.215891] bc041200: DMAMOV CCR 0x800201
[ 91.215898] bc041206: DMAMOV SAR 0x6d842000
[ 91.215903] bc04120c: DMAMOV DAR 0x12d30018
[ 91.215909] bc041212: DMALP_1 1
[ 91.215914] bc041214: DMAWFPS 5
[ 91.215920] bc041216: DMALDA
[ 91.215925] bc041217: DMASTPS 5
[ 91.215931] bc041219: DMAFLUSHP 5
[ 91.215936] bc04121b: DMALPENDA_1 bjmpto_7
[ 91.215948] bc04121d: DMASEV 1
[ 91.215959] bc04121f: DMAEND
[ 91.215974] dma-pl330 121b0000.pdma: setting up request on thread 0
[ 91.215978] bc041000: DMAMOV CCR 0x804200
[ 91.215984] bc041006: DMAMOV SAR 0x12d3001c
[ 91.215989] bc04100c: DMAMOV DAR 0x6d845000
[ 91.215994] bc041012: DMALP_1 1
[ 91.216000] bc041014: DMAWFPS 4
[ 91.216005] bc041016: DMALDPS 4
[ 91.216014] bc041018: DMASTA
[ 91.216023] bc041019: DMAFLUSHP 4
[ 91.216028] bc04101b: DMALPENDA_1 bjmpto_7
[ 91.216033] bc04101d: DMASEV 0
[ 91.216038] bc04101f: DMAEND
[ 91.216050] spi_master spi1: wait_for_dma: waiting for 100ms
transferring 2bytes@1000000Hz
[ 91.216055] dma-pl330 121b0000.pdma: event signalled on thread id 1
[ 91.216064] dma-pl330 121b0000.pdma: event signalled on thread id 0
[ 91.216579] spi_master spi1: wait_for_dma: waited 0 ms
[ 91.216655] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.216678] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
bpw 8 speed 1000000
[ 91.216684] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
src_clk sclk_spi1 mode bpw 8
[ 91.216716] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using dma
[ 91.216726] dma-pl330 121b0000.pdma: setting up request on thread 1
[ 91.216730] bc041200: DMAMOV CCR 0x800201
[ 91.216736] bc041206: DMAMOV SAR 0x6d842000
[ 91.216741] bc04120c: DMAMOV DAR 0x12d30018
[ 91.216747] bc041212: DMALP_1 3
[ 91.216752] bc041214: DMAWFPS 5
[ 91.216757] bc041216: DMALDA
[ 91.216762] bc041217: DMASTPS 5
[ 91.216767] bc041219: DMAFLUSHP 5
[ 91.216772] bc04121b: DMALPENDA_1 bjmpto_7
[ 91.216777] bc04121d: DMASEV 1
[ 91.216782] bc04121f: DMAEND
[ 91.216794] dma-pl330 121b0000.pdma: setting up request on thread 0
[ 91.216797] bc041000: DMAMOV CCR 0x804200
[ 91.216803] bc041006: DMAMOV SAR 0x12d3001c
[ 91.216808] bc04100c: DMAMOV DAR 0x6d845000
[ 91.216813] bc041012: DMALP_1 3
[ 91.216818] bc041014: DMAWFPS 4
[ 91.216823] bc041016: DMALDPS 4
[ 91.216828] bc041018: DMASTA
[ 91.216833] bc041019: DMAFLUSHP 4
[ 91.216838] bc04101b: DMALPENDA_1 bjmpto_7
[ 91.216844] bc04101d: DMASEV 0
[ 91.216849] bc04101f: DMAEND
[ 91.216860] spi_master spi1: wait_for_dma: waiting for 100ms
transferring 4bytes@1000000Hz
[ 91.216865] dma-pl330 121b0000.pdma: event signalled on thread id 1
[ 91.216891] dma-pl330 121b0000.pdma: event signalled on thread id 0
[ 91.216910] spi_master spi1: wait_for_dma: waited 0 ms
The parallel transfer of command and reply seems particualrly dodgy in
this case.
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
On 23 July 2015 at 18:46, Michal Suchanek <[email protected]> wrote:
> On 22 July 2015 at 11:01, Marek Vasut <[email protected]> wrote:
>> On Wednesday, July 22, 2015 at 10:38:14 AM, Michal Suchanek wrote:
>>> On 22 July 2015 at 10:24, Marek Vasut <[email protected]> wrote:
>>> > On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
>>> >> On 22 July 2015 at 09:58, Marek Vasut <[email protected]> wrote:
>>> >> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>>> >> >> On 22 July 2015 at 09:33, Marek Vasut <[email protected]> wrote:
>>> >> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>>> >> >> >> On 22 July 2015 at 06:49, Vinod Koul <[email protected]> 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
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 ?
On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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
On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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
On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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.
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.
> 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.
> - 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.
On 27 July 2015 at 19:43, Marek Vasut <[email protected]> wrote:
> On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
>> On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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.
>
>> 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.
>
>> - 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.
Thanks
Michal
On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
> On 27 July 2015 at 19:43, Marek Vasut <[email protected]> wrote:
> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> >> On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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 ?
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.
> >> 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 ?
> >> - 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 ?
On 30 July 2015 at 13:24, Marek Vasut <[email protected]> wrote:
> On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
>> On 27 July 2015 at 19:43, Marek Vasut <[email protected]> wrote:
>> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
>> >> On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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
On Thursday, July 30, 2015 at 02:18:09 PM, Michal Suchanek wrote:
> On 30 July 2015 at 13:24, Marek Vasut <[email protected]> wrote:
> > On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
> >> On 27 July 2015 at 19:43, Marek Vasut <[email protected]> wrote:
> >> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> >> >> On 24 July 2015 at 10:34, Marek Vasut <[email protected]> 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.
Aren't you forgetting that the data are synchronous to the clock which you
(your SPI block) generate ?
> > 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.
Might be a good idea. Check if the register is stuck for too long and
then zap the 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.
It would be a good idea to detail what your program exactly does on
the bus (like, "my program sends 3 bytes of data 0xf0 0x0b 0xar and
then receives 60 bytes of data"). Maybe I'm lost, but your test is
really not clear.
> >> >> 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.
That's not really important, is it ? If the transfer doesn't finish in, say,
1 second, and it is a 4096b transfer, then it is most likely timeout.
> 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.
OK
> >> >> - 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.
Good luck!