Enable access to SPI NOR flash on Samsung Snow board

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/arm/boot/dts/exynos5250-snow.dts +0 -0

index 1eca97e..1fa72cf 100644

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.

Quoted 5 lines · Michal Suchanek

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 ?

Quoted 9 lines · Marek Vasut

Quoted 5 lines · Michal Suchanek

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 at 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 at 0 {
        #address-cells = <1>;
        #size-cells = <1>;

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

        uimage at 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.

Quoted 11 lines · Michal Suchanek

Quoted 9 lines · Marek Vasut

Quoted 5 lines · Michal Suchanek

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 ?

Quoted 8 lines

This is how the DT entry for SPI slave looks with s3c64xx:

flash: m25p80 at 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.

Quoted 28 lines

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

this is example of flash partitions:

flash at 0 {
        #address-cells = <1>;
        #size-cells = <1>;

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

        uimage at 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.

Quoted 3 lines

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.

Quoted 3 lines

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

Quoted 63 lines · Marek Vasut

Quoted 14 lines · Michal Suchanek

Quoted 9 lines · Marek Vasut

Quoted 5 lines · Michal Suchanek

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 at 0 {

Quoted 6 lines

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

Quoted 37 lines

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

this is example of flash partitions:
flash at 0 {

        #address-cells = <1>;
        #size-cells = <1>;
        
        partition at 0 {
        
                label = "u-boot";
                reg = <0x0000000 0x100000>;
                read-only;
        
        };
        
        uimage at 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.

Quoted 8 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 5 lines

        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

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.

Signed-off-by Michal Suchanek <[email protected]>

---
 drivers/mtd/ofpart.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mtd/ofpart.c +2 -2

index aa26c32..a29d29f 100644
@@ -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;

Signature · 1 line

2.1.4

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.

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 <[email protected]>

--

Update commit message and documentation text.

---
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      |  6 ++++++
 drivers/mtd/devices/m25p80.c                       | 24

++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt index 2bee681..4e63ae8 100644

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +0 -0

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

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +2 -0

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

Quoted 6 lines · Michal Suchanek

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.

Quoted 27 lines

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 <[email protected]>

--

Update commit message and documentation text.

---
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      |  6 ++++++
 drivers/mtd/devices/m25p80.c                       | 24

++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-)

(quoted diff) +0 -0

b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt index 2bee681..4e63ae8 100644

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +0 -0

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

Quoted 6 lines

+- linux,max_tx_len : With some SPI controller drivers possible transfer
size is +                     limited. This may be hardware or driver bug.

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +2 -0

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

Quoted 8 lines · Marek Vasut

Quoted 6 lines · Michal Suchanek

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.

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 16 lines

Quoted 16 lines

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +0 -0

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

Quoted 5 lines

Quoted 2 lines

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

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.

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 <[email protected]>

--

Update commit message and documentation text.

---
 .../devicetree/bindings/mtd/jedec,spi-nor.txt      |  6 ++++++
 drivers/mtd/devices/m25p80.c                       | 24 ++++++++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt +0 -0

index 2bee681..4e63ae8 100644

Quoted 2 lines · Michal Suchanek

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.

Quoted 3 lines

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.

Quoted 6 lines

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.

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.

Quoted 7 lines · Michal Suchanek

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.

Quoted 7 lines · Richard Cochran

Quoted 6 lines · Michal Suchanek

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.

Quoted 3 lines

Quoted 1 line

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.

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.

Hi Michal,

Quoted 2 lines

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.

Quoted 3 lines · Brian Norris

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.

Quoted 4 lines · Marek Vasut

Quoted 3 lines · Brian Norris

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

Quoted 4 lines

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.

Quoted 2 lines

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

Hi Michal,

Quoted 2 lines · Marek Vasut

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.

Hello,

Quoted 15 lines · Brian Norris

Hi Michal,

Quoted 2 lines · Marek Vasut

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.

Quoted 5 lines

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?

Quoted 41 lines · Michal Suchanek

Hello,

Quoted 15 lines · Brian Norris

Hi Michal,

Quoted 2 lines · Marek Vasut

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.

Quoted 5 lines

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.

Quoted 6 lines

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

Quoted 13 lines

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

Quoted 1 line

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.

Quoted 15 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 4 lines

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.

Quoted 21 lines · Vinod Koul

Quoted 13 lines · Michal Suchanek

Quoted 13 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 4 lines

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

Quoted 27 lines · Michal Suchanek

Quoted 25 lines · Vinod Koul

Quoted 13 lines · Michal Suchanek

Quoted 13 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 4 lines

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.

Quoted 28 lines · Marek Vasut

Quoted 27 lines · Michal Suchanek

Quoted 25 lines · Vinod Koul

Quoted 13 lines · Michal Suchanek

Quoted 13 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 40 lines · Michal Suchanek

Quoted 28 lines · Marek Vasut

Quoted 27 lines · Michal Suchanek

Quoted 25 lines · Vinod Koul

Quoted 13 lines · Michal Suchanek

Quoted 13 lines

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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

Quoted 50 lines · Marek Vasut

Quoted 48 lines · Michal Suchanek

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Michal Suchanek

Quoted 28 lines · Vinod Koul

Quoted 14 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 54 lines · Michal Suchanek

Quoted 52 lines · Marek Vasut

Quoted 43 lines · Michal Suchanek

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Michal Suchanek

Quoted 28 lines · Vinod Koul

Quoted 14 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 4 lines

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.

Quoted 5 lines

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 ?

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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 ?

Quoted 61 lines · Marek Vasut

Quoted 59 lines · Michal Suchanek

Quoted 45 lines · Marek Vasut

Quoted 44 lines · Michal Suchanek

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Michal Suchanek

Quoted 28 lines · Vinod Koul

Quoted 14 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 16 lines

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 67 lines · Michal Suchanek

Quoted 65 lines · Marek Vasut

Quoted 52 lines · Michal Suchanek

Quoted 51 lines · Marek Vasut

Quoted 49 lines · Michal Suchanek

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Michal Suchanek

Quoted 28 lines · Vinod Koul

Quoted 14 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 2 lines

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!

Quoted 68 lines · Marek Vasut

Quoted 67 lines · Michal Suchanek

Quoted 65 lines · Marek Vasut

Quoted 52 lines · Michal Suchanek

Quoted 51 lines · Marek Vasut

Quoted 49 lines · Michal Suchanek

Quoted 31 lines · Marek Vasut

Quoted 30 lines · Michal Suchanek

Quoted 28 lines · Vinod Koul

Quoted 14 lines · Michal Suchanek

Quoted 14 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 6 lines

Quoted 2 lines

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

Quoted 45 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

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

Hi!

[...]

Quoted 50 lines

Quoted 45 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 4 lines

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

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

I have to admit, I don't quite understand the above. I also don't quite know what your spidev test does.

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

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

Hi!

[...]

Quoted 50 lines

Quoted 45 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 4 lines

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 ?

Quoted 57 lines · Marek Vasut

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

Hi!

[...]

Quoted 51 lines

Quoted 46 lines

Quoted 5 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 25 lines · Michal Suchanek

Quoted 2 lines · Marek Vasut

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 9 lines

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

Quoted 8 lines

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.

Quoted 29 lines · Marek Vasut

Quoted 28 lines · Michal Suchanek

Quoted 1 line · Marek Vasut

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 20 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 39 lines · Michal Suchanek

Quoted 37 lines · Marek Vasut

Quoted 24 lines · Michal Suchanek

Quoted 1 line · Marek Vasut

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 13 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 3 lines

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 ?

Quoted 21 lines

Quoted 21 lines

Quoted 9 lines

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

Quoted 8 lines

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.

Quoted 6 lines

Quoted 4 lines

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 ?

Quoted 46 lines · Marek Vasut

Quoted 44 lines · Michal Suchanek

Quoted 29 lines · Marek Vasut

Quoted 28 lines · Michal Suchanek

Quoted 1 line · Marek Vasut

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 17 lines

Quoted 16 lines

Quoted 9 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 27 lines

Quoted 26 lines

Quoted 20 lines

Quoted 8 lines

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

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 1 line

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.

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 <[email protected]>

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 <[email protected]>

The kernel is not trying to increase mcbufsz. It suggests you should try doing so. Also print the calculated required size of mcbufsz.

The SPI NOR transfers mysteriously fail so add more debug prints about SPI transactions.

Signed-off-by Michal Suchanek <[email protected]>

The SPI NOR transfers mysteriously fail so add more debug prints about SPI transactions.

drivers/spi/spi-s3c64xx.c line 21

Quoted 7 lines

drivers/spi/spi-s3c64xx.c +1 -0

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

Quoted 10 lines · Mark Brown

drivers/spi/spi-s3c64xx.c line 21

Quoted 7 lines

drivers/spi/spi-s3c64xx.c +1 -0

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

Quoted 2 lines · Michal Suchanek

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.

drivers/spi/spi-s3c64xx.c line 21

Quoted 8 lines

drivers/spi/spi-s3c64xx.c +1 -0

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

drivers/spi/spi-s3c64xx.c static int wait_for_dma(struct s3c64xx_spi_driver_data *sdd, · line 485

Quoted 5 lines

drivers/spi/spi-s3c64xx.c +4 -0

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

Quoted 3 lines · Mark Brown

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.

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.

Quoted 5 lines · Michal Suchanek

Quoted 3 lines · Mark Brown

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

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

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/arm/boot/dts/exynos5250-snow.dts +0 -0

index 1fa72cf..38e4cda 100644

Quoted 18 lines · Michal Suchanek

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 <[email protected]>

---

 arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos5250-snow.dts index 1fa72cf..38e4cda 100644

arch/arm/boot/dts/exynos5250-snow.dts line 696

Quoted 6 lines

arch/arm/boot/dts/exynos5250-snow.dts +1 -0

@@ -691,6 +691,18 @@
 
 	num-cs = <1>;
 	cs-gpios = <&gpa2 5 0>;
 	status = "okay";
 
+		flash: m25p80 at 0 {

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

arch/arm/boot/dts/exynos5250-snow.dts line 699

Quoted 5 lines

arch/arm/boot/dts/exynos5250-snow.dts +5 -0

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

Quoted 28 lines · Krzysztof Kozlowski

Quoted 18 lines · Michal Suchanek

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 <[email protected]>

---

 arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos5250-snow.dts index 1fa72cf..38e4cda 100644

arch/arm/boot/dts/exynos5250-snow.dts line 696

Quoted 6 lines

arch/arm/boot/dts/exynos5250-snow.dts +1 -0

@@ -691,6 +691,18 @@
 
 	num-cs = <1>;
 	cs-gpios = <&gpa2 5 0>;
 	status = "okay";
 
+		flash: m25p80 at 0 {

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

Quoted 8 lines

arch/arm/boot/dts/exynos5250-snow.dts line 699

Quoted 5 lines

arch/arm/boot/dts/exynos5250-snow.dts +5 -0

+			#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.

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 <[email protected]>

---
 arch/arm/boot/dts/exynos5250-snow.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

arch/arm/boot/dts/exynos5250-snow.dts +0 -0

index 1fa72cf..38e4cda 100644

Hello,

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.

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.

Quoted 1 line · Michal Suchanek

Hello,

Hi,

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 19 lines · Marek Vasut

Quoted 1 line · Michal Suchanek

Hello,

Hi,

Quoted 6 lines

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

Quoted 4 lines

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.

Quoted 1 line

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.