Re: [PATCH 00/11] Enable access to SPI NOR flash on Samsung Snow board

Re: [PATCH 00/11] Enable access to SPI NOR flash on Samsung Snow board

[Marek Vasut]

On Wednesday, June 03, 2015 at 11:26:39 PM, Michal Suchanek wrote:
> Hello,

Hi,

> this patch series makes it possible to access the SPI NOR flash in the
> Samsung XE303 'Snow' Chromebook.
> 
> Unfortunately not all issues are resolved. To work around an issue with the
> pl330 dma engine I respun the patch for limiting transfer size in m25p80
> driver.

Looks like fixing a bug at the wrong place to me ...

> As the flash does not contain any sane filesystem and is only likely to be
> accessed with mtd_debug or similar tool the limit of the dma engine is
> easily reached. Filesystems using shorter data transfers are less likely
> to be affected.

Sounds like the DMA engine driver should be fixed.

Best regards,
Marek Vasut

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Marek Vasut]

On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
> On Exynos it is necessary to set SPI controller parameters that apply to
> a SPI slave in a DT subnode of the slave device. The ofpart code returns
> an error when there are subnodes of the SPI flash but no partitions are
> found. Change this condition to a warning so that flash without
> partitions can be accessed on Exynos.

I have to admit the rationale for this patch is not very clear to me, sorry.
Can you please explain this a bit more ?

Best regards,
Marek Vasut

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

[Marek Vasut]

On Wednesday, June 03, 2015 at 11:26:41 PM, Michal Suchanek wrote:
> On sunxi the SPI controller currently does not have DMA support and fails
> any transfer larger than 63 bytes.
> 
> 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 smells more like some corruption of the data on the bus or something
even more obscure.

> 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.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> 
> --
> 
> 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)

Will we need similar patch for all other SPI slave drivers, like SPI NAND ?

Best regards,
Marek Vasut

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Marek Vasut]

On Wednesday, June 03, 2015 at 11:26:42 PM, Michal Suchanek wrote:
> The SPI NOR transfers mysteriously fail so add more debug prints about
> SPI transactions.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

dev_dbg() and friends would certainly be nicer here.

Best regards,
Marek Vasut

Re: [PATCH 11/11] dt: Exynos: add Snow SPI NOR node.

[Krzysztof Kozlowski]

On 04.06.2015 06:26, Michal Suchanek wrote:
> The Snow has onboard SPI NOR flash which contains the bootloader.
> 
> Add DT node for this flash chip. The flash is rated 133MHz but the pl330
> controller can transfer only up to 128 bytes at this speed so use more
> conservative settings. Even at 40MHz pl330 can transfer at most 64k with
> the current driver.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
> index 1fa72cf..38e4cda 100644
> --- a/arch/arm/boot/dts/exynos5250-snow.dts
> +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> @@ -691,6 +691,18 @@
>  	num-cs = <1>;
>  	cs-gpios = <&gpa2 5 0>;
>  	status = "okay";
> +		flash: m25p80@0 {

The indentation looks odd. This should be at the same level as "status".

> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			compatible = "jedec,spi-nor";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>;

So actually you wanted 133 MHz but as a workaround for DMA issue you use
40 MHz, right? Could you add here a small TODO note in comment about it?

Best regards,
Krzysztof

Re: [PATCH 01/11] ARM: dt: Add SPI CS on Samsung Snow board.

[Krzysztof Kozlowski]

On 04.06.2015 06:26, Michal Suchanek wrote:
> Although there is only one choice of chipselect it is necessary to
> specify it. The driver cannot claim the gpio otherwise.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Acked-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 09/11] dma: pl330: fix wording in mcbufsz message

[Krzysztof Kozlowski]

On 04.06.2015 06:26, Michal Suchanek wrote:
> The kernel is not trying to increase mcbufsz. It suggests you should try
> doing so. Also print the calculated required size of mcbufsz.
> 
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>

Looks good:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 00/11] Enable access to SPI NOR flash on Samsung Snow board

[Michal Suchanek]

On 4 June 2015 at 00:53, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, June 03, 2015 at 11:26:39 PM, Michal Suchanek wrote:
>> Hello,
>
> Hi,
>
>> this patch series makes it possible to access the SPI NOR flash in the
>> Samsung XE303 'Snow' Chromebook.
>>
>> Unfortunately not all issues are resolved. To work around an issue with the
>> pl330 dma engine I respun the patch for limiting transfer size in m25p80
>> driver.
>
> Looks like fixing a bug at the wrong place to me ...
>
>> As the flash does not contain any sane filesystem and is only likely to be
>> accessed with mtd_debug or similar tool the limit of the dma engine is
>> easily reached. Filesystems using shorter data transfers are less likely
>> to be affected.
>
> Sounds like the DMA engine driver should be fixed.

I looked at the pl330 datasheet and don't see anything obviusly wrong
with the pl330 driver in Linux.

Ideally it would be fixed but I have no idea what's wrong with it.

Thanks

Michal

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

[Michal Suchanek]

On 4 June 2015 at 01:03, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, June 03, 2015 at 11:26:41 PM, Michal Suchanek wrote:
>> On sunxi the SPI controller currently does not have DMA support and fails
>> any transfer larger than 63 bytes.
>>
>> 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 smells more like some corruption of the data on the bus or something
> even more obscure.

If the data was corrupted you would get corrupted data and not
transfer failure. AFAIK there is no checksum or anything. I actually
do get corrupted data when I use wrong feedback delay setting.

>
>> 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.
>>
>> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
>>
>> --
>>
>> 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)
>
> Will we need similar patch for all other SPI slave drivers, like SPI NAND ?

Probably. Some SPI slave drivers already do have similar option. In
general it cannot be expected that you can reliably transfer
arbitrarily large data over SPI it seems. However, if the nand driver
transfers data a page at a time it should be fine.

Thanks

Michal

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Michal Suchanek]

On 4 June 2015 at 00:58, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
>> On Exynos it is necessary to set SPI controller parameters that apply to
>> a SPI slave in a DT subnode of the slave device. The ofpart code returns
>> an error when there are subnodes of the SPI flash but no partitions are
>> found. Change this condition to a warning so that flash without
>> partitions can be accessed on Exynos.
>
> I have to admit the rationale for this patch is not very clear to me, sorry.
> Can you please explain this a bit more ?

This is how the DT entry for SPI slave looks with s3c64xx:
flash: m25p80@0 {
        #address-cells = <1>;
        #size-cells = <1>;
        compatible = "jedec,spi-nor";
        reg = <0>;
        spi-max-frequency = <40000000>;
        linux,max_tx_len = <65536>;
        m25p,fast-read;
        controller-data {
            samsung,spi-feedback-delay = <0>;
        };
    };

this is example of flash partitions:
flash@0 {
        #address-cells = <1>;
        #size-cells = <1>;

        partition@0 {
                label = "u-boot";
                reg = <0x0000000 0x100000>;
                read-only;
        };

        uimage@100000 {
                reg = <0x0100000 0x200000>;
        };
};

The parser ignores any flash without subnodes and returns 0 (no
partititon). When there is a subnode it assumes the flash is
partitioned and tries to parse the subnodes as partitions. When there
are subnodes and none parses as partition an error is returned. As
shown above it is valid to have subnodes on unpartitioned flash.

When an error is returned from a partition parser the mtdpart code
passes on this error to the flash probe function and the proble of the
flash fails.

Thanks

Michal

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

[Geert Uytterhoeven]

On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek <hramrach@gmail.com> 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 <hramrach@gmail.com>
>
> --
>
> 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 -- geert@linux-m68k.org

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

Re: [PATCH 01/11] ARM: dt: Add SPI CS on Samsung Snow board.

[Javier Martinez Canillas]

Hello Michal,

On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek <hramrach@gmail.com> wrote:
> Although there is only one choice of chipselect it is necessary to
> specify it. The driver cannot claim the gpio otherwise.
>
> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250-snow.dts b/arch/arm/boot/dts/exynos5250-snow.dts
> index 1eca97e..1fa72cf 100644
> --- a/arch/arm/boot/dts/exynos5250-snow.dts
> +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> @@ -687,9 +687,10 @@
>  };
>
>  &spi_1 {
> -       status = "okay";
>         samsung,spi-src-clk = <0>;
>         num-cs = <1>;
> +       cs-gpios = <&gpa2 5 0>;
> +       status = "okay";

Why are you changing the position of the status property? AFAIK the
convention is to have this property at the beginning of the device
node.

Best regards,
Javier

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

[Michal Suchanek]

On 4 June 2015 at 08:42, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Jun 3, 2015 at 11:26 PM, Michal Suchanek <hramrach@gmail.com> 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

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Mark Brown]

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

On Wed, Jun 03, 2015 at 09:26:42PM -0000, Michal Suchanek wrote:
> The SPI NOR transfers mysteriously fail so add more debug prints about
> SPI transactions.

Please try to only send patches to relevant people - the list of
recipients for this is so large that it only barely fits on a single
screen in my mail client.

Also for this patch (which just adds some trace) there isn't any clear
reason for it to be sent as part of the series at all, it doesn't help
deliver the functionality and doesn't depend on the rest of the series.

> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -18,6 +18,7 @@
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>

Whatever you're doing here this indicates that there's a very big
abstraction problem going on.

> +	pr_debug("%s %s %s waiting for %ims transferring %zubytes@%iHz",
> +		 __func__, sdd->pdev ? dev_name(&sdd->pdev->dev) : NULL,
> +		 dev_name(&sdd->master->dev),
> +		 ms, xfer->len, sdd->cur_speed);

I'd say dev_dbg() but more generally this is just tracing things that
seem to be already covered by the trace points already present in the
core, the same goes for most of the rest of it.  If there's things
missing from the existing trace it seems better to add to it.

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

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Geert Uytterhoeven]

On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown <broonie@kernel.org> wrote:
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/delay.h>
>>  #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> Whatever you're doing here this indicates that there's a very big
> abstraction problem going on.

I guess it's needed for __clk_get_name(), which can be avoided by using
the "%pC" printf format specifier instead.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Michal Suchanek]

Hello,

On 4 June 2015 at 11:16, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 03, 2015 at 09:26:42PM -0000, Michal Suchanek wrote:
>> The SPI NOR transfers mysteriously fail so add more debug prints about
>> SPI transactions.
>
> Please try to only send patches to relevant people - the list of
> recipients for this is so large that it only barely fits on a single
> screen in my mail client.
>
> Also for this patch (which just adds some trace) there isn't any clear
> reason for it to be sent as part of the series at all, it doesn't help
> deliver the functionality and doesn't depend on the rest of the series.

I used this patch to add missing information to dmesg output so I
could diagnose the SPI failures so I think it is relevant.

The failure is reported by the s3c64xx but the root cause is pl330 not
finishing a DMA transfer.

It is non-obvious that this is the issue. The information that a call
to pl330 failed is only available in the s3c64xx driver and not SPI
core so I think there is no reasonable way to debug this issue other
than adding prints in s3c64xx. This issue is not completely resolved
or even well tested (I have only single board to test with) so these
prints remain relevant in my view.

>
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/delay.h>
>>  #include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>
> Whatever you're doing here this indicates that there's a very big
> abstraction problem going on.

I wanted to print a clock name in case the information was relevant
for the issue at hand.

To print a clock name you AFAICT need this header. I think this is an
abstraction problem in the clock framework. Fixing the abstraction
problem with clock framework would only grow the list of recipients
which is already so long you complain about it.

It turns out that in this case the clock was not at fault so I did not
need the clock name in the end.

>
>> +     pr_debug("%s %s %s waiting for %ims transferring %zubytes@%iHz",
>> +              __func__, sdd->pdev ? dev_name(&sdd->pdev->dev) : NULL,
>> +              dev_name(&sdd->master->dev),
>> +              ms, xfer->len, sdd->cur_speed);
>
> I'd say dev_dbg() but more generally this is just tracing things that
> seem to be already covered by the trace points already present in the
> core, the same goes for most of the rest of it.  If there's things
> missing from the existing trace it seems better to add to it.

There is master and slave device involved but it's certainly possible
to use dev_dbg on one of them and pass the other as string.

Thanks

Michal

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Mark Brown]

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

On Thu, Jun 04, 2015 at 11:30:15AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 4, 2015 at 11:16 AM, Mark Brown <broonie@kernel.org> wrote:
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -18,6 +18,7 @@
> >>  #include <linux/interrupt.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/clk.h>
> >> +#include <linux/clk-provider.h>

> > Whatever you're doing here this indicates that there's a very big
> > abstraction problem going on.

> I guess it's needed for __clk_get_name(), which can be avoided by using
> the "%pC" printf format specifier instead.

Right, or by adding an externally usable API - the point is that using
internal functions in a client driver is a big red flag.

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

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Mark Brown]

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

On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote:
> On 4 June 2015 at 11:16, Mark Brown <broonie@kernel.org> wrote:

> > Also for this patch (which just adds some trace) there isn't any clear
> > reason for it to be sent as part of the series at all, it doesn't help
> > deliver the functionality and doesn't depend on the rest of the series.

> I used this patch to add missing information to dmesg output so I
> could diagnose the SPI failures so I think it is relevant.

The fact that you used this to debug problems is not relevant to any fix
you might have developed for those problems; the fix can be explained
without needing to know how you got there.  Similarly the debugging is
hopefully useful in general without needing to know which particular
problem prompted it's creation.

> To print a clock name you AFAICT need this header. I think this is an
> abstraction problem in the clock framework. Fixing the abstraction
> problem with clock framework would only grow the list of recipients
> which is already so long you complain about it.

This is a bit of a warning sign that the series isn't very focused.
It's also just not good practice, sending patches with obvious problems
(especially obvious problems that aren't clearly flagged up as something
temporary) will frustrate reviewers and can often lead to other issues
being obscured.

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

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Michal Suchanek]

On 4 June 2015 at 12:26, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jun 04, 2015 at 11:33:37AM +0200, Michal Suchanek wrote:
>> On 4 June 2015 at 11:16, Mark Brown <broonie@kernel.org> wrote:
>
>> > Also for this patch (which just adds some trace) there isn't any clear
>> > reason for it to be sent as part of the series at all, it doesn't help
>> > deliver the functionality and doesn't depend on the rest of the series.
>
>> I used this patch to add missing information to dmesg output so I
>> could diagnose the SPI failures so I think it is relevant.
>
> The fact that you used this to debug problems is not relevant to any fix
> you might have developed for those problems; the fix can be explained
> without needing to know how you got there.  Similarly the debugging is
> hopefully useful in general without needing to know which particular
> problem prompted it's creation.

Yes, this patch should be meaningful on its own.

The ultimate purpose of this patch series is to make the SPI NOR flash
chip usable on boards of which I have 1 sample. If the results on
boards other than mine happen to be different the debug prints will be
useful for users of this flash chip.

There is no code interdependency with the other patches so if it's
preferred I can send patches like this separately.

>
>> To print a clock name you AFAICT need this header. I think this is an
>> abstraction problem in the clock framework. Fixing the abstraction
>> problem with clock framework would only grow the list of recipients
>> which is already so long you complain about it.
>
> This is a bit of a warning sign that the series isn't very focused.
> It's also just not good practice, sending patches with obvious problems
> (especially obvious problems that aren't clearly flagged up as something
> temporary) will frustrate reviewers and can often lead to other issues
> being obscured.

As has been pointed out public interface already exists for getting
the clock name so the issue here is cosmetic.

Thanks

Michal

Re: [PATCH 10/11] spi: add more debug prints in s3c64xx

[Mark Brown]

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

On Thu, Jun 04, 2015 at 12:52:19PM +0200, Michal Suchanek wrote:

> There is no code interdependency with the other patches so if it's
> preferred I can send patches like this separately.

Yes, that's better practice.

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

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

[Marek Vasut]

On Thursday, June 04, 2015 at 06:31:45 AM, Michal Suchanek wrote:
> On 4 June 2015 at 01:03, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, June 03, 2015 at 11:26:41 PM, Michal Suchanek wrote:
> >> On sunxi the SPI controller currently does not have DMA support and
> >> fails any transfer larger than 63 bytes.
> >> 
> >> 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 smells more like some corruption of the data on the bus or something
> > even more obscure.
> 
> If the data was corrupted you would get corrupted data and not
> transfer failure. AFAIK there is no checksum or anything. I actually
> do get corrupted data when I use wrong feedback delay setting.

OK

> >> The best thing is that in both cases the controller can just lock up and
> >> never finish potentially leaving the hardware in unusable state.

[...]

> >> --- 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)
> > 
> > Will we need similar patch for all other SPI slave drivers, like SPI NAND
> > ?
> 
> Probably. Some SPI slave drivers already do have similar option.

So the SPI device drivers are implementing workarounds for buggy SPI hosts,
even if those SPI devices themselves don't suffer from such limitations. Yes,
this seems like the seriously wrong thing to do.

> In general it cannot be expected that you can reliably transfer
> arbitrarily large data over SPI it seems.

This is what should be expected because this is how the bus is supposed
to work in fact. You can transfer an arbitrary amount of data over SPI
bus.

If some driver has a limitation, it's that (bus, dma) driver which needs
to implement a fix or a workaround.

> However, if the nand driver transfers data a page at a time it should
> be fine.

... until someone issues multi-block read, in which case it all falls
down like a house of cards. It is impossible to build a reliable system
on "should" and similar assumptions ; the driver must be solid.

Best regards,
Marek Vasut

Re: [PATCH 11/11] dt: Exynos: add Snow SPI NOR node.

[Marek Vasut]

On Thursday, June 04, 2015 at 04:04:18 AM, Krzysztof Kozlowski wrote:
> On 04.06.2015 06:26, Michal Suchanek wrote:
> > The Snow has onboard SPI NOR flash which contains the bootloader.
> > 
> > Add DT node for this flash chip. The flash is rated 133MHz but the pl330
> > controller can transfer only up to 128 bytes at this speed so use more
> > conservative settings. Even at 40MHz pl330 can transfer at most 64k with
> > the current driver.
> > 
> > Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> > ---
> > 
> >  arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/exynos5250-snow.dts
> > b/arch/arm/boot/dts/exynos5250-snow.dts index 1fa72cf..38e4cda 100644
> > --- a/arch/arm/boot/dts/exynos5250-snow.dts
> > +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> > @@ -691,6 +691,18 @@
> > 
> >  	num-cs = <1>;
> >  	cs-gpios = <&gpa2 5 0>;
> >  	status = "okay";
> > 
> > +		flash: m25p80@0 {
> 
> The indentation looks odd. This should be at the same level as "status".
> 
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			compatible = "jedec,spi-nor";
> > +			reg = <0>;
> > +			spi-max-frequency = <40000000>;
> 
> So actually you wanted 133 MHz but as a workaround for DMA issue you use
> 40 MHz, right? Could you add here a small TODO note in comment about it?

I disagree. There is a problem, the problem should actually be analyzed and 
fixed, not just postponed with some TODO nonsense.

Best regards,
Marek Vasut

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Marek Vasut]

On Thursday, June 04, 2015 at 06:54:00 AM, Michal Suchanek wrote:
> On 4 June 2015 at 00:58, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
> >> On Exynos it is necessary to set SPI controller parameters that apply to
> >> a SPI slave in a DT subnode of the slave device. The ofpart code returns
> >> an error when there are subnodes of the SPI flash but no partitions are
> >> found. Change this condition to a warning so that flash without
> >> partitions can be accessed on Exynos.
> > 
> > I have to admit the rationale for this patch is not very clear to me,
> > sorry. Can you please explain this a bit more ?
> 
> This is how the DT entry for SPI slave looks with s3c64xx:
> flash: m25p80@0 {
>         #address-cells = <1>;
>         #size-cells = <1>;
>         compatible = "jedec,spi-nor";
>         reg = <0>;
>         spi-max-frequency = <40000000>;
>         linux,max_tx_len = <65536>;

SIDENOTE: I thought this was actually added by your patch #8 in this
series. The underscores in the name of the property are not really
consistent with the rest of the names.

>         m25p,fast-read;
>         controller-data {
>             samsung,spi-feedback-delay = <0>;
>         };
>     };
> 
> this is example of flash partitions:
> flash@0 {
>         #address-cells = <1>;
>         #size-cells = <1>;
> 
>         partition@0 {
>                 label = "u-boot";
>                 reg = <0x0000000 0x100000>;
>                 read-only;
>         };
> 
>         uimage@100000 {
>                 reg = <0x0100000 0x200000>;
>         };
> };
> 
> The parser ignores any flash without subnodes and returns 0 (no
> partititon). When there is a subnode it assumes the flash is
> partitioned and tries to parse the subnodes as partitions. When there
> are subnodes and none parses as partition an error is returned. As
> shown above it is valid to have subnodes on unpartitioned flash.
>
> When an error is returned from a partition parser the mtdpart code
> passes on this error to the flash probe function and the proble of the
> flash fails.

What does /proc/mtd tell you when you have no partitions defined
in the DT ? It should provide you with the entire MTD device and
the code shouldn't even try to parse any OF partitions, since you
don't have any.

Best regards,
Marek Vasut

Re: [PATCH 00/11] Enable access to SPI NOR flash on Samsung Snow board

[Marek Vasut]

On Thursday, June 04, 2015 at 06:21:32 AM, Michal Suchanek wrote:
> On 4 June 2015 at 00:53, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, June 03, 2015 at 11:26:39 PM, Michal Suchanek wrote:
> >> Hello,
> > 
> > Hi,
> > 
> >> this patch series makes it possible to access the SPI NOR flash in the
> >> Samsung XE303 'Snow' Chromebook.
> >> 
> >> Unfortunately not all issues are resolved. To work around an issue with
> >> the pl330 dma engine I respun the patch for limiting transfer size in
> >> m25p80 driver.
> > 
> > Looks like fixing a bug at the wrong place to me ...
> > 
> >> As the flash does not contain any sane filesystem and is only likely to
> >> be accessed with mtd_debug or similar tool the limit of the dma engine
> >> is easily reached. Filesystems using shorter data transfers are less
> >> likely to be affected.
> > 
> > Sounds like the DMA engine driver should be fixed.
> 
> I looked at the pl330 datasheet and don't see anything obviusly wrong
> with the pl330 driver in Linux.
> 
> Ideally it would be fixed but I have no idea what's wrong with it.

Ask maybe Catalin from ARM to help you -- he might be able to point you
to the right hardware guy.

Best regards,
Marek Vasut

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Michal Suchanek]

On 4 June 2015 at 17:28, Marek Vasut <marex@denx.de> wrote:
> On Thursday, June 04, 2015 at 06:54:00 AM, Michal Suchanek wrote:
>> On 4 June 2015 at 00:58, Marek Vasut <marex@denx.de> wrote:
>> > On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
>> >> On Exynos it is necessary to set SPI controller parameters that apply to
>> >> a SPI slave in a DT subnode of the slave device. The ofpart code returns
>> >> an error when there are subnodes of the SPI flash but no partitions are
>> >> found. Change this condition to a warning so that flash without
>> >> partitions can be accessed on Exynos.
>> >
>> > I have to admit the rationale for this patch is not very clear to me,
>> > sorry. Can you please explain this a bit more ?
>>
>> This is how the DT entry for SPI slave looks with s3c64xx:
>> flash: m25p80@0 {
>>         #address-cells = <1>;
>>         #size-cells = <1>;
>>         compatible = "jedec,spi-nor";
>>         reg = <0>;
>>         spi-max-frequency = <40000000>;
>>         linux,max_tx_len = <65536>;
>
> SIDENOTE: I thought this was actually added by your patch #8 in this
> series. The underscores in the name of the property are not really
> consistent with the rest of the names.
>
>>         m25p,fast-read;
>>         controller-data {
>>             samsung,spi-feedback-delay = <0>;
>>         };
>>     };
>>
>> this is example of flash partitions:
>> flash@0 {
>>         #address-cells = <1>;
>>         #size-cells = <1>;
>>
>>         partition@0 {
>>                 label = "u-boot";
>>                 reg = <0x0000000 0x100000>;
>>                 read-only;
>>         };
>>
>>         uimage@100000 {
>>                 reg = <0x0100000 0x200000>;
>>         };
>> };
>>
>> The parser ignores any flash without subnodes and returns 0 (no
>> partititon). When there is a subnode it assumes the flash is
>> partitioned and tries to parse the subnodes as partitions. When there
>> are subnodes and none parses as partition an error is returned. As
>> shown above it is valid to have subnodes on unpartitioned flash.
>>
>> When an error is returned from a partition parser the mtdpart code
>> passes on this error to the flash probe function and the proble of the
>> flash fails.
>
> What does /proc/mtd tell you when you have no partitions defined
> in the DT ? It should provide you with the entire MTD device and
> the code shouldn't even try to parse any OF partitions, since you
> don't have any.

mtdinfo shows I have no mtd devices and the log shows that probe
failed unless I patch the kernel.

When there is *support* for of partitions the ofparser is run on mtd
probe. If it fails because it considers

>>         controller-data {
>>             samsung,spi-feedback-delay = <0>;
>>         };

an invalid partition and does not find any valid partition the probe fails.

There are two problems here

1) the above is not invalid. It just is not a partition. The parser
should not fail seeing this

2) the mtd probe should not fail when a partition parser fails and
should present the unpartitioned device

Both are addressed in separate patches.

Thanks

Michal

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

[Richard Cochran]

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

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Marek Vasut]

On Thursday, June 04, 2015 at 05:40:58 PM, Michal Suchanek wrote:
> On 4 June 2015 at 17:28, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, June 04, 2015 at 06:54:00 AM, Michal Suchanek wrote:
> >> On 4 June 2015 at 00:58, Marek Vasut <marex@denx.de> wrote:
> >> > On Wednesday, June 03, 2015 at 11:26:40 PM, Michal Suchanek wrote:
> >> >> On Exynos it is necessary to set SPI controller parameters that apply
> >> >> to a SPI slave in a DT subnode of the slave device. The ofpart code
> >> >> returns an error when there are subnodes of the SPI flash but no
> >> >> partitions are found. Change this condition to a warning so that
> >> >> flash without partitions can be accessed on Exynos.
> >> > 
> >> > I have to admit the rationale for this patch is not very clear to me,
> >> > sorry. Can you please explain this a bit more ?
> >> 
> >> This is how the DT entry for SPI slave looks with s3c64xx:
> >> flash: m25p80@0 {
> >> 
> >>         #address-cells = <1>;
> >>         #size-cells = <1>;
> >>         compatible = "jedec,spi-nor";
> >>         reg = <0>;
> >>         spi-max-frequency = <40000000>;
> >>         linux,max_tx_len = <65536>;
> > 
> > SIDENOTE: I thought this was actually added by your patch #8 in this
> > series. The underscores in the name of the property are not really
> > consistent with the rest of the names.
> > 
> >>         m25p,fast-read;
> >>         controller-data {
> >>         
> >>             samsung,spi-feedback-delay = <0>;
> >>         
> >>         };
> >>     
> >>     };
> >> 
> >> this is example of flash partitions:
> >> flash@0 {
> >> 
> >>         #address-cells = <1>;
> >>         #size-cells = <1>;
> >>         
> >>         partition@0 {
> >>         
> >>                 label = "u-boot";
> >>                 reg = <0x0000000 0x100000>;
> >>                 read-only;
> >>         
> >>         };
> >>         
> >>         uimage@100000 {
> >>         
> >>                 reg = <0x0100000 0x200000>;
> >>         
> >>         };
> >> 
> >> };
> >> 
> >> The parser ignores any flash without subnodes and returns 0 (no
> >> partititon). When there is a subnode it assumes the flash is
> >> partitioned and tries to parse the subnodes as partitions. When there
> >> are subnodes and none parses as partition an error is returned. As
> >> shown above it is valid to have subnodes on unpartitioned flash.
> >> 
> >> When an error is returned from a partition parser the mtdpart code
> >> passes on this error to the flash probe function and the proble of the
> >> flash fails.
> > 
> > What does /proc/mtd tell you when you have no partitions defined
> > in the DT ? It should provide you with the entire MTD device and
> > the code shouldn't even try to parse any OF partitions, since you
> > don't have any.
> 
> mtdinfo shows I have no mtd devices and the log shows that probe
> failed unless I patch the kernel.
> 
> When there is *support* for of partitions the ofparser is run on mtd
> probe. If it fails because it considers
> 
> >>         controller-data {
> >>         
> >>             samsung,spi-feedback-delay = <0>;
> >>         
> >>         };
> 
> an invalid partition and does not find any valid partition the probe fails.

OK, now it has become clearer to me, thanks!

> There are two problems here
> 
> 1) the above is not invalid. It just is not a partition. The parser
> should not fail seeing this

The parser should complain verbosely and ignore this I guess ?

> 2) the mtd probe should not fail when a partition parser fails and
> should present the unpartitioned device

OK

> Both are addressed in separate patches.
> 
> Thanks
> 
> Michal

Re: [PATCH 09/11] dma: pl330: fix wording in mcbufsz message

[Vinod Koul]

On Wed, Jun 03, 2015 at 09:26:41PM +0000, Michal Suchanek wrote:
> The kernel is not trying to increase mcbufsz. It suggests you should try
> doing so. Also print the calculated required size of mcbufsz.
pls use right subsystem name in the patches

I have applied this now

-- 
~Vinod

Re: [PATCH 11/11] dt: Exynos: add Snow SPI NOR node.

[Pavel Machek]

On Thu 2015-06-04 17:20:54, Marek Vasut wrote:
> On Thursday, June 04, 2015 at 04:04:18 AM, Krzysztof Kozlowski wrote:
> > On 04.06.2015 06:26, Michal Suchanek wrote:
> > > The Snow has onboard SPI NOR flash which contains the bootloader.
> > > 
> > > Add DT node for this flash chip. The flash is rated 133MHz but the pl330
> > > controller can transfer only up to 128 bytes at this speed so use more
> > > conservative settings. Even at 40MHz pl330 can transfer at most 64k with
> > > the current driver.
> > > 
> > > Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> > > ---
> > > 
> > >  arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/exynos5250-snow.dts
> > > b/arch/arm/boot/dts/exynos5250-snow.dts index 1fa72cf..38e4cda 100644
> > > --- a/arch/arm/boot/dts/exynos5250-snow.dts
> > > +++ b/arch/arm/boot/dts/exynos5250-snow.dts
> > > @@ -691,6 +691,18 @@
> > > 
> > >  	num-cs = <1>;
> > >  	cs-gpios = <&gpa2 5 0>;
> > >  	status = "okay";
> > > 
> > > +		flash: m25p80@0 {
> > 
> > The indentation looks odd. This should be at the same level as "status".
> > 
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			compatible = "jedec,spi-nor";
> > > +			reg = <0>;
> > > +			spi-max-frequency = <40000000>;
> > 
> > So actually you wanted 133 MHz but as a workaround for DMA issue you use
> > 40 MHz, right? Could you add here a small TODO note in comment about it?
> 
> I disagree. There is a problem, the problem should actually be analyzed and 
> fixed, not just postponed with some TODO nonsense.

Of course, that would be preferable if submitter has time. But OTOH
slow SPI is probably better than no SPI at all, and debugging this can
be nasty. I think that TODO is suitable.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 04/11] mtd: ofpart: do not fail probe when no partitions exist

[Brian Norris]

On Wed, Jun 03, 2015 at 09:26:40PM -0000, Michal Suchanek wrote:
> On Exynos it is necessary to set SPI controller parameters that apply to
> a SPI slave in a DT subnode of the slave device. The ofpart code returns
> an error when there are subnodes of the SPI flash but no partitions are
> found. Change this condition to a warning so that flash without
> partitions can be accessed on Exynos.

So judging by the subsequent discussion, you're looking at handling
nodes like this:

	flash@... {
		compatible = "m25p80";
		...

		controller-data {
			samsung,spi-feedback-delay = <0>;
		};
	};

Now, I'm not a real fan of this controller-data node in the first place
(did that binding get reviewed?). But this is especially bad since
we now have collisions on what to do with subnodes that don't have a
compatible property. By legacy, ofpart has already claimed ownership of
subnodes of an MTD node, where the subnode does not have a compatible
property. See Documentation/devicetree/bindings/mtd/partition.txt:

  "NOTE: if the sub-node has a compatible string, then it is not a
  partition."

So it seems the natural solution is to just define a proper
compatibile property for this subnode, and ofpart.c will naturally
handle this. See:

commit e79265ba6bdb31437bd4c3e7911950f9d1262a07
Author: Josh Wu <josh.wu@atmel.com>
Date:   Mon Aug 5 19:14:38 2013 +0800

    mtd: ofpart: add compatible check for child nodes



Brian

> Signed-off-by: Michal Suchanek <hramrach@gmail.com>
> ---
>  drivers/mtd/ofpart.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index aa26c32..a29d29f 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -94,10 +94,10 @@ static int parse_ofpart_partitions(struct mtd_info *master,
>  
>  	if (!i) {
>  		of_node_put(pp);
> -		pr_err("No valid partition found on %s\n", node->full_name);
> +		pr_warn("No valid partition found on %s\n", node->full_name);
>  		kfree(*pparts);
>  		*pparts = NULL;
> -		return -EINVAL;
> +		return 0;
>  	}
>  
>  	return nr_parts;
> -- 
> 2.1.4
> 

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

[Michal Suchanek]

On 4 June 2015 at 19:15, Richard Cochran <richardcochran@gmail.com> 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

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

[Marek Vasut]

On Wednesday, July 15, 2015 at 11:45:07 AM, Michal Suchanek wrote:
> On 4 June 2015 at 19:15, Richard Cochran <richardcochran@gmail.com> 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

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

[Brian Norris]

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

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

[Marek Vasut]

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

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

[Brian Norris]

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

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

[Marek Vasut]

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

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

[Michal Suchanek]

Hello,

On 15 July 2015 at 17:59, Brian Norris <computersforpeace@gmail.com> 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

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

[Vinod Koul]

On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote:
> Hello,
> 
> On 15 July 2015 at 17:59, Brian Norris <computersforpeace@gmail.com> 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

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

[Michal Suchanek]

Hello,

On 21 July 2015 at 06:29, Vinod Koul <vinod.koul@intel.com> wrote:
> On Sun, Jul 19, 2015 at 09:01:34PM +0200, Michal Suchanek wrote:
>> Hello,
>>
>> On 15 July 2015 at 17:59, Brian Norris <computersforpeace@gmail.com> 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

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

[Vinod Koul]

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

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

[Michal Suchanek]

On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>> > Or alternatively we could publish the limitations of the channel using
>> > capabilities so SPI knows I have a dmaengine channel and it can transfer max N
>> > length transfers so would be able to break rather than guessing it or coding
>> > in DT. Yes it may come from DT but that should be dmaengine driver rather
>> > than client driver :)
>> >
>> > This can be done by dma_get_slave_caps(chan, &caps)
>> >
>> > And we add max_length as one more parameter to existing set
>> >
>> > Also all this could be handled in generic SPI-dmaengine layer so that
>> > individual drivers don't have to code it in
>> >
>> > Let me know if this idea is okay, I can push the dmaengine bits...
>>
>> It would be ok if there was a fixed limit. However, the limit depends
>> on SPI slave settings. Presumably for other buses using the dmaengine
>> the limit would depend on the bus or slave settings as well. I do not
>> see a sane way of passing this all the way to the dmaengine driver.
> I don't see why this should be client (SPI) dependent. The max length
> supported is a dmaengine constraint, typically flowing from max
> blocks/length it can transfer. Know this limit can allow clients to split
> transfers.

In practice on the board I have the maximum transfer length before it
fails depends on SPI bus speed which is set up per slave. I did not
try searching the space of possible settings thorougly and settled for
a setting that gives reasonable speed and transfer length.

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

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

[Marek Vasut]

On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
> >> > Or alternatively we could publish the limitations of the channel using
> >> > capabilities so SPI knows I have a dmaengine channel and it can
> >> > transfer max N length transfers so would be able to break rather than
> >> > guessing it or coding in DT. Yes it may come from DT but that should
> >> > be dmaengine driver rather than client driver :)
> >> > 
> >> > This can be done by dma_get_slave_caps(chan, &caps)
> >> > 
> >> > And we add max_length as one more parameter to existing set
> >> > 
> >> > Also all this could be handled in generic SPI-dmaengine layer so that
> >> > individual drivers don't have to code it in
> >> > 
> >> > Let me know if this idea is okay, I can push the dmaengine bits...
> >> 
> >> It would be ok if there was a fixed limit. However, the limit depends
> >> on SPI slave settings. Presumably for other buses using the dmaengine
> >> the limit would depend on the bus or slave settings as well. I do not
> >> see a sane way of passing this all the way to the dmaengine driver.
> > 
> > I don't see why this should be client (SPI) dependent. The max length
> > supported is a dmaengine constraint, typically flowing from max
> > blocks/length it can transfer. Know this limit can allow clients to split
> > transfers.
> 
> In practice on the board I have the maximum transfer length before it
> fails depends on SPI bus speed which is set up per slave. I did not
> try searching the space of possible settings thorougly and settled for
> a setting that gives reasonable speed and transfer length.

This looks more like a signal integrity issue though.

> 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

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

[Michal Suchanek]

On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
>> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>> >> > Or alternatively we could publish the limitations of the channel using
>> >> > capabilities so SPI knows I have a dmaengine channel and it can
>> >> > transfer max N length transfers so would be able to break rather than
>> >> > guessing it or coding in DT. Yes it may come from DT but that should
>> >> > be dmaengine driver rather than client driver :)
>> >> >
>> >> > This can be done by dma_get_slave_caps(chan, &caps)
>> >> >
>> >> > And we add max_length as one more parameter to existing set
>> >> >
>> >> > Also all this could be handled in generic SPI-dmaengine layer so that
>> >> > individual drivers don't have to code it in
>> >> >
>> >> > Let me know if this idea is okay, I can push the dmaengine bits...
>> >>
>> >> It would be ok if there was a fixed limit. However, the limit depends
>> >> on SPI slave settings. Presumably for other buses using the dmaengine
>> >> the limit would depend on the bus or slave settings as well. I do not
>> >> see a sane way of passing this all the way to the dmaengine driver.
>> >
>> > I don't see why this should be client (SPI) dependent. The max length
>> > supported is a dmaengine constraint, typically flowing from max
>> > blocks/length it can transfer. Know this limit can allow clients to split
>> > transfers.
>>
>> In practice on the board I have the maximum transfer length before it
>> fails depends on SPI bus speed which is set up per slave. I did not
>> try searching the space of possible settings thorougly and settled for
>> a setting that gives reasonable speed and transfer length.
>
> This looks more like a signal integrity issue though.
>

It certainly does on the surface. However, when wrong data is
delivered over the SPI bus (such as when I use wrong phase setting)
the SPI controller happily delivers wrong data over PIO.

The failure I am seeing is that the pl330 DMA program which repeatedly
waits for data from the SPI controller never finishes the read loop
and does not signal the interrupt. It seems it also leaves some data
in a FIFO somewhere so next command on the flash returns garbage and
fails.

Thanks

Michal

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

[Marek Vasut]

On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
> On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> >> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
> >> >> > Or alternatively we could publish the limitations of the channel
> >> >> > using capabilities so SPI knows I have a dmaengine channel and it
> >> >> > can transfer max N length transfers so would be able to break
> >> >> > rather than guessing it or coding in DT. Yes it may come from DT
> >> >> > but that should be dmaengine driver rather than client driver :)
> >> >> > 
> >> >> > This can be done by dma_get_slave_caps(chan, &caps)
> >> >> > 
> >> >> > And we add max_length as one more parameter to existing set
> >> >> > 
> >> >> > Also all this could be handled in generic SPI-dmaengine layer so
> >> >> > that individual drivers don't have to code it in
> >> >> > 
> >> >> > Let me know if this idea is okay, I can push the dmaengine bits...
> >> >> 
> >> >> It would be ok if there was a fixed limit. However, the limit depends
> >> >> on SPI slave settings. Presumably for other buses using the dmaengine
> >> >> the limit would depend on the bus or slave settings as well. I do not
> >> >> see a sane way of passing this all the way to the dmaengine driver.
> >> > 
> >> > I don't see why this should be client (SPI) dependent. The max length
> >> > supported is a dmaengine constraint, typically flowing from max
> >> > blocks/length it can transfer. Know this limit can allow clients to
> >> > split transfers.
> >> 
> >> In practice on the board I have the maximum transfer length before it
> >> fails depends on SPI bus speed which is set up per slave. I did not
> >> try searching the space of possible settings thorougly and settled for
> >> a setting that gives reasonable speed and transfer length.
> > 
> > This looks more like a signal integrity issue though.
> 
> It certainly does on the surface. However, when wrong data is
> delivered over the SPI bus (such as when I use wrong phase setting)
> the SPI controller happily delivers wrong data over PIO.
> 
> The failure I am seeing is that the pl330 DMA program which repeatedly
> waits for data from the SPI controller never finishes the read loop
> and does not signal the interrupt. It seems it also leaves some data
> in a FIFO somewhere so next command on the flash returns garbage and
> fails.

I observed something similar on MXS (mx28) SPI block. Do you use mixed
PIO/DMA mode perhaps ? 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

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

[Michal Suchanek]

On 22 July 2015 at 09:58, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
>> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> >> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
>> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>> >> >> > Or alternatively we could publish the limitations of the channel
>> >> >> > using capabilities so SPI knows I have a dmaengine channel and it
>> >> >> > can transfer max N length transfers so would be able to break
>> >> >> > rather than guessing it or coding in DT. Yes it may come from DT
>> >> >> > but that should be dmaengine driver rather than client driver :)
>> >> >> >
>> >> >> > This can be done by dma_get_slave_caps(chan, &caps)
>> >> >> >
>> >> >> > And we add max_length as one more parameter to existing set
>> >> >> >
>> >> >> > Also all this could be handled in generic SPI-dmaengine layer so
>> >> >> > that individual drivers don't have to code it in
>> >> >> >
>> >> >> > Let me know if this idea is okay, I can push the dmaengine bits...
>> >> >>
>> >> >> It would be ok if there was a fixed limit. However, the limit depends
>> >> >> on SPI slave settings. Presumably for other buses using the dmaengine
>> >> >> the limit would depend on the bus or slave settings as well. I do not
>> >> >> see a sane way of passing this all the way to the dmaengine driver.
>> >> >
>> >> > I don't see why this should be client (SPI) dependent. The max length
>> >> > supported is a dmaengine constraint, typically flowing from max
>> >> > blocks/length it can transfer. Know this limit can allow clients to
>> >> > split transfers.
>> >>
>> >> In practice on the board I have the maximum transfer length before it
>> >> fails depends on SPI bus speed which is set up per slave. I did not
>> >> try searching the space of possible settings thorougly and settled for
>> >> a setting that gives reasonable speed and transfer length.
>> >
>> > This looks more like a signal integrity issue though.
>>
>> It certainly does on the surface. However, when wrong data is
>> delivered over the SPI bus (such as when I use wrong phase setting)
>> the SPI controller happily delivers wrong data over PIO.
>>
>> The failure I am seeing is that the pl330 DMA program which repeatedly
>> waits for data from the SPI controller never finishes the read loop
>> and does not signal the interrupt. It seems it also leaves some data
>> in a FIFO somewhere so next command on the flash returns garbage and
>> fails.
>
> I observed something similar on MXS (mx28) SPI block. Do you use mixed
> PIO/DMA mode perhaps ?

The SPI driver uses PIO for short transfers and DMA for transfers
longer than the controller FIFO. This seems to be the standard way to
do things.It works flawlessly so long as submitting overly long DMA
programs is avoided.

> 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

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

[Marek Vasut]

On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
> On 22 July 2015 at 09:58, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
> >> On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
> >> >> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
> >> >> >> > Or alternatively we could publish the limitations of the channel
> >> >> >> > using capabilities so SPI knows I have a dmaengine channel and
> >> >> >> > it can transfer max N length transfers so would be able to
> >> >> >> > break rather than guessing it or coding in DT. Yes it may come
> >> >> >> > from DT but that should be dmaengine driver rather than client
> >> >> >> > driver :)
> >> >> >> > 
> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps)
> >> >> >> > 
> >> >> >> > And we add max_length as one more parameter to existing set
> >> >> >> > 
> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer so
> >> >> >> > that individual drivers don't have to code it in
> >> >> >> > 
> >> >> >> > Let me know if this idea is okay, I can push the dmaengine
> >> >> >> > bits...
> >> >> >> 
> >> >> >> It would be ok if there was a fixed limit. However, the limit
> >> >> >> depends on SPI slave settings. Presumably for other buses using
> >> >> >> the dmaengine the limit would depend on the bus or slave settings
> >> >> >> as well. I do not see a sane way of passing this all the way to
> >> >> >> the dmaengine driver.
> >> >> > 
> >> >> > I don't see why this should be client (SPI) dependent. The max
> >> >> > length supported is a dmaengine constraint, typically flowing from
> >> >> > max blocks/length it can transfer. Know this limit can allow
> >> >> > clients to split transfers.
> >> >> 
> >> >> In practice on the board I have the maximum transfer length before it
> >> >> fails depends on SPI bus speed which is set up per slave. I did not
> >> >> try searching the space of possible settings thorougly and settled
> >> >> for a setting that gives reasonable speed and transfer length.
> >> > 
> >> > This looks more like a signal integrity issue though.
> >> 
> >> It certainly does on the surface. However, when wrong data is
> >> delivered over the SPI bus (such as when I use wrong phase setting)
> >> the SPI controller happily delivers wrong data over PIO.
> >> 
> >> The failure I am seeing is that the pl330 DMA program which repeatedly
> >> waits for data from the SPI controller never finishes the read loop
> >> and does not signal the interrupt. It seems it also leaves some data
> >> in a FIFO somewhere so next command on the flash returns garbage and
> >> fails.
> > 
> > I observed something similar on MXS (mx28) SPI block. Do you use mixed
> > PIO/DMA mode perhaps ?
> 
> The SPI driver uses PIO for short transfers and DMA for transfers
> longer than the controller FIFO. This seems to be the standard way to
> do things.It works flawlessly so long as submitting overly long DMA
> programs is avoided.

Can you try doing JUST DMA, no PIO ? I remember seeing some bus synchronisation
issues when I did mixed PIO/DMA on the MXS and it was nasty to track down. Just
give pure DMA a go to see if the thing stabilizes somehow.

> > 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 ?

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

[Michal Suchanek]

On 22 July 2015 at 10:24, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, July 22, 2015 at 10:18:04 AM, Michal Suchanek wrote:
>> On 22 July 2015 at 09:58, Marek Vasut <marex@denx.de> wrote:
>> > On Wednesday, July 22, 2015 at 09:45:27 AM, Michal Suchanek wrote:
>> >> On 22 July 2015 at 09:33, Marek Vasut <marex@denx.de> wrote:
>> >> > On Wednesday, July 22, 2015 at 09:30:54 AM, Michal Suchanek wrote:
>> >> >> On 22 July 2015 at 06:49, Vinod Koul <vinod.koul@intel.com> wrote:
>> >> >> > On Tue, Jul 21, 2015 at 10:14:11AM +0200, Michal Suchanek wrote:
>> >> >> >> > Or alternatively we could publish the limitations of the channel
>> >> >> >> > using capabilities so SPI knows I have a dmaengine channel and
>> >> >> >> > it can transfer max N length transfers so would be able to
>> >> >> >> > break rather than guessing it or coding in DT. Yes it may come
>> >> >> >> > from DT but that should be dmaengine driver rather than client
>> >> >> >> > driver :)
>> >> >> >> >
>> >> >> >> > This can be done by dma_get_slave_caps(chan, &caps)
>> >> >> >> >
>> >> >> >> > And we add max_length as one more parameter to existing set
>> >> >> >> >
>> >> >> >> > Also all this could be handled in generic SPI-dmaengine layer so
>> >> >> >> > that individual drivers don't have to code it in
>> >> >> >> >
>> >> >> >> > Let me know if this idea is okay, I can push the dmaengine
>> >> >> >> > bits...
>> >> >> >>
>> >> >> >> It would be ok if there was a fixed limit. However, the limit
>> >> >> >> depends on SPI slave settings. Presumably for other buses using
>> >> >> >> the dmaengine the limit would depend on the bus or slave settings
>> >> >> >> as well. I do not see a sane way of passing this all the way to
>> >> >> >> the dmaengine driver.
>> >> >> >
>> >> >> > I don't see why this should be client (SPI) dependent. The max
>> >> >> > length supported is a dmaengine constraint, typically flowing from
>> >> >> > max blocks/length it can transfer. Know this limit can allow
>> >> >> > clients to split transfers.
>> >> >>
>> >> >> In practice on the board I have the maximum transfer length before it
>> >> >> fails depends on SPI bus speed which is set up per slave. I did not
>> >> >> try searching the space of possible settings thorougly and settled
>> >> >> for a setting that gives reasonable speed and transfer length.
>> >> >
>> >> > This looks more like a signal integrity issue though.
>> >>
>> >> It certainly does on the surface. However, when wrong data is
>> >> delivered over the SPI bus (such as when I use wrong phase setting)
>> >> the SPI controller happily delivers wrong data over PIO.
>> >>
>> >> The failure I am seeing is that the pl330 DMA program which repeatedly
>> >> waits for data from the SPI controller never finishes the read loop
>> >> and does not signal the interrupt. It seems it also leaves some data
>> >> in a FIFO somewhere so next command on the flash returns garbage and
>> >> fails.
>> >
>> > I observed something similar on MXS (mx28) SPI block. Do you use mixed
>> > PIO/DMA mode perhaps ?
>>
>> The SPI driver uses PIO for short transfers and DMA for transfers
>> longer than the controller FIFO. This seems to be the standard way to
>> do things.It works flawlessly so long as submitting overly long DMA
>> programs is avoided.
>
> Can you try doing JUST DMA, no PIO ? I remember seeing some bus synchronisation
> issues when I did mixed PIO/DMA on the MXS and it was nasty to track down. Just
> give pure DMA a go to see if the thing stabilizes somehow.

It's probably slower to set up DMA for 2-byte commands but it might
work nonetheless.

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

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

[Marek Vasut]

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

It is, the overhead will be considerable. It might help the stability
though. I'm really looking forward to the results!

> 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

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

[Michal Suchanek]

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

Hello,

this does not quite work.

My test with spidev:

# ./spinor /dev/spidev1.0
Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60

I receive correct ID but spi-nor complains it does not know ID 00 c8 60.
IIRC garbage should be sent only at the time command is transferred so
only one byte of garbage should be received. Also the garbage tends to
be the last state of the data output - all 0 or all 1.
So it seems using DMA for all transfers including 1-byte commands
results in (some?) received data getting an extra 00 prefix.

[   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

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

[Michal Suchanek]

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

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

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

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

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

Thanks

Michal

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

[Marek Vasut]

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 ?

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

[Michal Suchanek]

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

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

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

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

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

Thanks

Michal

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

[Michal Suchanek]

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

Ok, so here is some summary.

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

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

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

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

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

It turns out that

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

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

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

Thanks

Michal

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

[Marek Vasut]

On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
> > 
> > Hi!
> > 
> > [...]
> > 
> >> >>> It's probably slower to set up DMA for 2-byte commands but it might
> >> >>> work nonetheless.
> >> >> 
> >> >> It is, the overhead will be considerable. It might help the stability
> >> >> though. I'm really looking forward to the results!
> >> > 
> >> > Hello,
> >> > 
> >> > this does not quite work.
> >> > 
> >> > My test with spidev:
> >> > 
> >> > # ./spinor /dev/spidev1.0
> >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
> >> > Sending 90 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> > Received 00 ff ff ff ff c8 15 c8 15 c8 15 c8 15 c8 15 c8
> >> > Sending 9f 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >> > Received 00 ff c8 60 16 c8 60 16 c8 60 16 c8 60 16 c8 60
> >> > 
> >> > I receive correct ID but spi-nor complains it does not know ID 00 c8
> >> > 60. IIRC garbage should be sent only at the time command is
> >> > transferred so only one byte of garbage should be received. Also the
> >> > garbage tends to be the last state of the data output - all 0 or all
> >> > 1.
> >> > So it seems using DMA for all transfers including 1-byte commands
> >> > results in (some?) received data getting an extra 00 prefix.
> >> > 
> >> > 
> >> > I also managed to lock up the controller completely since there is
> >> > some error passing the SPI speed somewhere :(
> >> > 
> >> > [ 1352.977530] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max
> >> > --> 0 [ 1352.977540] spidev spi1.0: spi mode 0
> >> > [ 1352.977576] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max
> >> > --> 0 [ 1352.977582] spidev spi1.0: msb first
> >> > [ 1352.977614] spidev spi1.0: setup mode 0, 8 bits/w, 80000000 Hz max
> >> > --> 0 [ 1352.977620] spidev spi1.0: 0 bits per word
> >> > [ 1352.977652] spidev spi1.0: setup mode 0, 8 bits/w, 2690588672 Hz
> >> > max --> 0 [ 1352.977726] spi_master spi1: s3c64xx_spi_config:
> >> > clk_from_cmu 1 src_clk sclk_spi1 mode bpw 8
> >> > [ 1352.977753] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: xfer
> >> > bpw 8 speed -1604378624
> >> > [ 1352.977760] spi_master spi1: s3c64xx_spi_config: clk_from_cmu 1
> >> > src_clk sclk_spi1 mode bpw 8
> >> > [ 1352.977781] spi_master spi1: spi1.0 s3c64xx_spi_transfer_one: using
> >> > dma [ 1352.977797] dma-pl330 121b0000.pdma: setting up request on
> >> > thread 1
> >> 
> >> Hmm, on a second thought it probably works as expected more or less.
> >> 
> >> The nonsensical value was passed from application and there is no
> >> guard against that.
> >> 
> >> Since I don't do PIO the controller remains locked up indefinitely.
> > 
> > I have to admit, I don't quite understand the above. I also don't quite
> > know what your spidev test does. Can you possibly just bind a regular
> > SPI NOR driver and run mtdtests to see if it is stable ?
> 
> Ok, so here is some summary.
> 
> I have a NOR flash attached to a s3c64xx SPI controller with 64byte
> fifo and a pl330 dma controller.
> 
> Normally DMA controller is used for transfers that do not fit into the
> FIFO.
> 
> I tried adding the flash memory ID to the spi-nor driver table and
> adding a DT node for it.
> 
> The flash is rated at 120MHz so I used that speed but the ID was
> bit-shifted and identification failed. There is DT property
> samsung,spi-feedback-delay for addressing this and at 120MHz it must
> be 2 or 3 on this board. 40MHz works with default value 0.
> 
> The next step after identification worked was to try reading the flash
> content. For this the DMA controller is used because data is
> transferred in blocks larger than 64 bytes. When reading the whole 4MB
> flash the transfer failed silently. I got a 4MB file of all ones or
> all zeroes.
> 
> It turns out that
> 
>  - the pl330 locks up when transfering large amount of data.
> Specifically, the maximum power of 2 sized transfer at 120MHz is 128
> bytes and 64k at 40MHz. Transferring more than this results in the
> pl330 locking up and never signalling completion of the transfer.

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.

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

[Michal Suchanek]

On 27 July 2015 at 19:43, Marek Vasut <marex@denx.de> wrote:
> On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
>> On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
>> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
>> >
>> Ok, so here is some summary.
>>
>> I have a NOR flash attached to a s3c64xx SPI controller with 64byte
>> fifo and a pl330 dma controller.
>>
>> Normally DMA controller is used for transfers that do not fit into the
>> FIFO.
>>
>> I tried adding the flash memory ID to the spi-nor driver table and
>> adding a DT node for it.
>>
>> The flash is rated at 120MHz so I used that speed but the ID was
>> bit-shifted and identification failed. There is DT property
>> samsung,spi-feedback-delay for addressing this and at 120MHz it must
>> be 2 or 3 on this board. 40MHz works with default value 0.
>>
>> The next step after identification worked was to try reading the flash
>> content. For this the DMA controller is used because data is
>> transferred in blocks larger than 64 bytes. When reading the whole 4MB
>> flash the transfer failed silently. I got a 4MB file of all ones or
>> all zeroes.
>>
>> It turns out that
>>
>>  - the pl330 locks up when transfering large amount of data.
>> Specifically, the maximum power of 2 sized transfer at 120MHz is 128
>> bytes and 64k at 40MHz. Transferring more than this results in the
>> pl330 locking up and never signalling completion of the transfer.
>
> Hypothesis:
> I think this might just be that the controller didn't catch all the
> inbound clock ticks and thus counted less inbound data than it was
> set up to receive. The controller thus waits for more data.

The flash chip can transfer data as long as you keep the clock going,
especially when you transfer 64k off a 4M flash.

The read command is bound just by stopping the clock. There is always
more data to be had.

>
>> 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

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

[Marek Vasut]

On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
> On 27 July 2015 at 19:43, Marek Vasut <marex@denx.de> wrote:
> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> >> On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
> >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
> >> Ok, so here is some summary.
> >> 
> >> I have a NOR flash attached to a s3c64xx SPI controller with 64byte
> >> fifo and a pl330 dma controller.
> >> 
> >> Normally DMA controller is used for transfers that do not fit into the
> >> FIFO.
> >> 
> >> I tried adding the flash memory ID to the spi-nor driver table and
> >> adding a DT node for it.
> >> 
> >> The flash is rated at 120MHz so I used that speed but the ID was
> >> bit-shifted and identification failed. There is DT property
> >> samsung,spi-feedback-delay for addressing this and at 120MHz it must
> >> be 2 or 3 on this board. 40MHz works with default value 0.
> >> 
> >> The next step after identification worked was to try reading the flash
> >> content. For this the DMA controller is used because data is
> >> transferred in blocks larger than 64 bytes. When reading the whole 4MB
> >> flash the transfer failed silently. I got a 4MB file of all ones or
> >> all zeroes.
> >> 
> >> It turns out that
> >> 
> >>  - the pl330 locks up when transfering large amount of data.
> >> 
> >> Specifically, the maximum power of 2 sized transfer at 120MHz is 128
> >> bytes and 64k at 40MHz. Transferring more than this results in the
> >> pl330 locking up and never signalling completion of the transfer.
> > 
> > Hypothesis:
> > I think this might just be that the controller didn't catch all the
> > inbound clock ticks and thus counted less inbound data than it was
> > set up to receive. The controller thus waits for more data.
> 
> The flash chip can transfer data as long as you keep the clock going,
> especially when you transfer 64k off a 4M flash.
> 
> The read command is bound just by stopping the clock. There is always
> more data to be had.

Sure, if you keep clocking the chip, the data will keep going. Is the
chip being clocked ?

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 ?

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

[Michal Suchanek]

On 30 July 2015 at 13:24, Marek Vasut <marex@denx.de> wrote:
> On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
>> On 27 July 2015 at 19:43, Marek Vasut <marex@denx.de> wrote:
>> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
>> >> On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
>> >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
>> >> Ok, so here is some summary.
>> >>
>> >> I have a NOR flash attached to a s3c64xx SPI controller with 64byte
>> >> fifo and a pl330 dma controller.
>> >>
>> >> Normally DMA controller is used for transfers that do not fit into the
>> >> FIFO.
>> >>
>> >> I tried adding the flash memory ID to the spi-nor driver table and
>> >> adding a DT node for it.
>> >>
>> >> The flash is rated at 120MHz so I used that speed but the ID was
>> >> bit-shifted and identification failed. There is DT property
>> >> samsung,spi-feedback-delay for addressing this and at 120MHz it must
>> >> be 2 or 3 on this board. 40MHz works with default value 0.
>> >>
>> >> The next step after identification worked was to try reading the flash
>> >> content. For this the DMA controller is used because data is
>> >> transferred in blocks larger than 64 bytes. When reading the whole 4MB
>> >> flash the transfer failed silently. I got a 4MB file of all ones or
>> >> all zeroes.
>> >>
>> >> It turns out that
>> >>
>> >>  - the pl330 locks up when transfering large amount of data.
>> >>
>> >> Specifically, the maximum power of 2 sized transfer at 120MHz is 128
>> >> bytes and 64k at 40MHz. Transferring more than this results in the
>> >> pl330 locking up and never signalling completion of the transfer.
>> >
>> > Hypothesis:
>> > I think this might just be that the controller didn't catch all the
>> > inbound clock ticks and thus counted less inbound data than it was
>> > set up to receive. The controller thus waits for more data.
>>
>> The flash chip can transfer data as long as you keep the clock going,
>> especially when you transfer 64k off a 4M flash.
>>
>> The read command is bound just by stopping the clock. There is always
>> more data to be had.
>
> Sure, if you keep clocking the chip, the data will keep going. Is the
> chip being clocked ?

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

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

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

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

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

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

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

>
>> >> Data
>> >> is left in FIFO which causes subsequent commands to fail since garbage
>> >> is returned instead of command reply. Also subsequent read may cause
>> >> I/O error or or return an empty page depending on the garbage around.
>> >
>> > So the driver for the DMA controller might need to be augmented to handle
>> > this case -- add a timeout and in case DMA times out, drain the FIFO to
>> > make sure subsequent transfers do not fail.
>>
>> There is no way to add timeout in the DMA driver since it does not
>> know the SPI clock.
>>
>> The timeout is handled by the SPI driver and it should be possible to
>> augment it to drain the FIFO when DMA fails so long as FIFO level is
>> readable somewhere.
>
> If the DMA doesn't complete in certain amount of time, it should time out
> I'd say. Don't you think ?

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

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

>
>> >> - the I/O errors are not checked in spi-nor at all which leads to
>> >> silent data corruption.
>> >>
>> >> On a suggestion that this may improve reliability I changed the
>> >> s3c64xx driver to use DMA for all transfers. This caused
>> >> identification to fail in spin-nor because the ID was prefixed with
>> >> extra 00  byte. Testing with spidev confirmed that everything is
>> >> prefixed with extra 00.
>> >
>> > The determinism of this is in fact excellent news.
>> >
>> >> Also when the DMA controller locked up no
>> >> transfers were possible anymore. When DMA was not used for sending
>> >> commands the controller would recover on next command. I made the
>> >> option to always use DMA configurable and it turns out that the
>> >> returned data is prefixed with 00 only when no transfer without DMA
>> >> was ever made. Loading the spi-nor driver with the dma-only option off
>> >> and then with dma-only option on results in correct operation. Only
>> >> loading the dma-only driver first causes the 00 prefix.
>> >
>> > Can we conclude that the PIO codepath somehow programs a register
>> > somewhere which gets rid of this 0x00 prefix ? If so, this should then
>> > also be part of the DMA codepath, or even better, this should be set in
>> > probe() somewhere.
>>
>> Yes, it looks like it.
>
> Did you find what this could be please ?

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

I did not get to dissecting it so far.

Thanks

Michal

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

[Marek Vasut]

On Thursday, July 30, 2015 at 02:18:09 PM, Michal Suchanek wrote:
> On 30 July 2015 at 13:24, Marek Vasut <marex@denx.de> wrote:
> > On Monday, July 27, 2015 at 10:43:05 PM, Michal Suchanek wrote:
> >> On 27 July 2015 at 19:43, Marek Vasut <marex@denx.de> wrote:
> >> > On Monday, July 27, 2015 at 11:46:25 AM, Michal Suchanek wrote:
> >> >> On 24 July 2015 at 10:34, Marek Vasut <marex@denx.de> wrote:
> >> >> > On Thursday, July 23, 2015 at 07:03:47 PM, Michal Suchanek wrote:
> >> >> Ok, so here is some summary.
> >> >> 
> >> >> I have a NOR flash attached to a s3c64xx SPI controller with 64byte
> >> >> fifo and a pl330 dma controller.
> >> >> 
> >> >> Normally DMA controller is used for transfers that do not fit into
> >> >> the FIFO.
> >> >> 
> >> >> I tried adding the flash memory ID to the spi-nor driver table and
> >> >> adding a DT node for it.
> >> >> 
> >> >> The flash is rated at 120MHz so I used that speed but the ID was
> >> >> bit-shifted and identification failed. There is DT property
> >> >> samsung,spi-feedback-delay for addressing this and at 120MHz it must
> >> >> be 2 or 3 on this board. 40MHz works with default value 0.
> >> >> 
> >> >> The next step after identification worked was to try reading the
> >> >> flash content. For this the DMA controller is used because data is
> >> >> transferred in blocks larger than 64 bytes. When reading the whole
> >> >> 4MB flash the transfer failed silently. I got a 4MB file of all ones
> >> >> or all zeroes.
> >> >> 
> >> >> It turns out that
> >> >> 
> >> >>  - the pl330 locks up when transfering large amount of data.
> >> >> 
> >> >> Specifically, the maximum power of 2 sized transfer at 120MHz is 128
> >> >> bytes and 64k at 40MHz. Transferring more than this results in the
> >> >> pl330 locking up and never signalling completion of the transfer.
> >> > 
> >> > Hypothesis:
> >> > I think this might just be that the controller didn't catch all the
> >> > inbound clock ticks and thus counted less inbound data than it was
> >> > set up to receive. The controller thus waits for more data.
> >> 
> >> The flash chip can transfer data as long as you keep the clock going,
> >> especially when you transfer 64k off a 4M flash.
> >> 
> >> The read command is bound just by stopping the clock. There is always
> >> more data to be had.
> > 
> > Sure, if you keep clocking the chip, the data will keep going. Is the
> > chip being clocked ?
> 
> There is always data. Like in whenever you want to read SPI data you
> sample the input pin and watever you sample is data so far as SPI bus
> is concerned.

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!