sunxi spi fixes

The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over 1MHz SPI bus takes way longer than that. Calculate the timeout from the actual time the transfer is supposed to take and multiply by 2 for good measure.

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

---

v2:
- fix build error
- use unsigned instead of int in max_t
- use tfr->speed_hz instead of sspi->max_speed_hz

---
 drivers/spi/spi-sun4i.c | 11 ++++++++++-
 drivers/spi/spi-sun6i.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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

index 1ddd9e2..fe63bbd 100644
@@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 {
        struct sun4i_spi *sspi = spi_master_get_devdata(master);
        unsigned int mclk_rate, div, timeout;
+       unsigned int start, end, tx_time;
        unsigned int tx_len = 0;
        int ret = 0;
        u32 reg;

drivers/spi/spi-sun4i.c +0 -0

                ret = -ETIMEDOUT;
                goto out;
        }

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

index 42e2c4b..8be5c5c 100644
@@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
        unsigned int mclk_rate, div, timeout;
        unsigned int tx_len = 0;
        int ret = 0;
+       unsigned int start, end, tx_time;
        u32 reg;
 
        /* We don't support transfer larger than the FIFO */

Hi Michal,

Quoted 33 lines · Michal Suchanek

The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over 1MHz SPI bus takes way longer than that. Calculate the timeout from the actual time the transfer is supposed to take and multiply by 2 for good measure.

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

---

v2:
- fix build error
- use unsigned instead of int in max_t
- use tfr->speed_hz instead of sspi->max_speed_hz

---
 drivers/spi/spi-sun4i.c | 11 ++++++++++-
 drivers/spi/spi-sun6i.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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

index 1ddd9e2..fe63bbd 100644
@@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 {
        struct sun4i_spi *sspi = spi_master_get_devdata(master);
        unsigned int mclk_rate, div, timeout;
+       unsigned int start, end, tx_time;
        unsigned int tx_len = 0;
        int ret = 0;
        u32 reg;

@@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
        reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
        sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
 

drivers/spi/spi-sun4i.c static int sun4i_spi_transfer_one(struct spi_master *master, · line 283

Quoted 1 line

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

+       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

You should probably use "unsigned int" instead of just "unsigned" here.

Quoted 1 line

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

+                       100);

drivers/spi/spi-sun4i.c static int sun4i_spi_transfer_one(struct spi_master *master, · line 293

Quoted 10 lines

drivers/spi/spi-sun4i.c +7 -1

+       start = jiffies;
        timeout = wait_for_completion_timeout(&sspi->done,
-                                             msecs_to_jiffies(1000));
+                                             msecs_to_jiffies(tx_time));
+       end = jiffies;
        if (!timeout) {
+               dev_warn(&master->dev,
+                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
+                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
+                        jiffies_to_msecs(end - start), tx_time);

Should the debug changes be in a separate patch?

Quoted 17 lines

drivers/spi/spi-sun4i.c +0 -0

                ret = -ETIMEDOUT;
                goto out;
        }

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

index 42e2c4b..8be5c5c 100644
@@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
        unsigned int mclk_rate, div, timeout;
        unsigned int tx_len = 0;
        int ret = 0;
+       unsigned int start, end, tx_time;
        u32 reg;
 
        /* We don't support transfer larger than the FIFO */

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 273

Quoted 4 lines

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

@@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
        reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
        sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
 
+       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

Ditto, "unsigned int" instead of "unsigned"?

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 283

Quoted 11 lines

drivers/spi/spi-sun6i.c +8 -1

+                       100);
+       start = jiffies;
        timeout = wait_for_completion_timeout(&sspi->done,
-                                             msecs_to_jiffies(1000));
+                                             msecs_to_jiffies(tx_time));
+       end = jiffies;
        if (!timeout) {
+               dev_warn(&master->dev,
+                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
+                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
+                        jiffies_to_msecs(end - start), tx_time);

Ditto, separate patch?

Also, should the changes for the drivers be in two separate patches also?

Quoted 36 lines · Julian Calaby

Hi Michal,

Quoted 33 lines · Michal Suchanek

The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over 1MHz SPI bus takes way longer than that. Calculate the timeout from the actual time the transfer is supposed to take and multiply by 2 for good measure.

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

---

v2:
- fix build error
- use unsigned instead of int in max_t
- use tfr->speed_hz instead of sspi->max_speed_hz

---
 drivers/spi/spi-sun4i.c | 11 ++++++++++-
 drivers/spi/spi-sun6i.c | 11 ++++++++++-
 2 files changed, 20 insertions(+), 2 deletions(-)

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

index 1ddd9e2..fe63bbd 100644
@@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 {
        struct sun4i_spi *sspi = spi_master_get_devdata(master);
        unsigned int mclk_rate, div, timeout;
+       unsigned int start, end, tx_time;
        unsigned int tx_len = 0;
        int ret = 0;
        u32 reg;

@@ -279,9 +280,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
        reg = sun4i_spi_read(sspi, SUN4I_CTL_REG);
        sun4i_spi_write(sspi, SUN4I_CTL_REG, reg | SUN4I_CTL_XCH);
 

Quoted 5 lines

drivers/spi/spi-sun4i.c static int sun4i_spi_transfer_one(struct spi_master *master, · line 283

Quoted 1 line

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

+       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

You should probably use "unsigned int" instead of just "unsigned" here.

Quoted 1 line

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

+                       100);

Or just 100U constant and avoid max_t altogether.

Quoted 11 lines

Quoted 10 lines

drivers/spi/spi-sun4i.c +7 -1

+       start = jiffies;
        timeout = wait_for_completion_timeout(&sspi->done,
-                                             msecs_to_jiffies(1000));
+                                             msecs_to_jiffies(tx_time));
+       end = jiffies;
        if (!timeout) {
+               dev_warn(&master->dev,
+                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
+                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
+                        jiffies_to_msecs(end - start), tx_time);

Quoted 1 line

Should the debug changes be in a separate patch?

Is this so big of a change that it needs to be split?

Quoted 37 lines

Quoted 18 lines

drivers/spi/spi-sun4i.c +0 -0

                ret = -ETIMEDOUT;
                goto out;
        }

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

index 42e2c4b..8be5c5c 100644
@@ -162,6 +162,7 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
        unsigned int mclk_rate, div, timeout;
        unsigned int tx_len = 0;
        int ret = 0;
+       unsigned int start, end, tx_time;
        u32 reg;
 
        /* We don't support transfer larger than the FIFO */

drivers/spi/spi-sun6i.c static int sun6i_spi_transfer_one(struct spi_master *master, · line 273

Quoted 4 lines

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

@@ -269,9 +270,17 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
        reg = sun6i_spi_read(sspi, SUN6I_TFR_CTL_REG);
        sun6i_spi_write(sspi, SUN6I_TFR_CTL_REG, reg | SUN6I_TFR_CTL_XCH);
 
+       tx_time = max_t(unsigned, tfr->len * 8 * 2 / (tfr->speed_hz / 1000),

Ditto, "unsigned int" instead of "unsigned"?

Quoted 11 lines

drivers/spi/spi-sun6i.c +8 -1

+                       100);
+       start = jiffies;
        timeout = wait_for_completion_timeout(&sspi->done,
-                                             msecs_to_jiffies(1000));
+                                             msecs_to_jiffies(tx_time));
+       end = jiffies;
        if (!timeout) {
+               dev_warn(&master->dev,
+                        "%s: timeout transferring %u bytes@%iHz for %i(%i)ms",
+                        dev_name(&spi->dev), tfr->len, tfr->speed_hz,
+                        jiffies_to_msecs(end - start), tx_time);

Quoted 3 lines

Ditto, separate patch?

Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Quoted 3 lines · Michal Suchanek

Quoted 1 line

Also, should the changes for the drivers be in two separate patches also?

That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

Quoted 3 lines · Julian Calaby

Quoted 1 line · Michal Suchanek

Quoted 1 line

Also, should the changes for the drivers be in two separate patches also?

Quoted 3 lines

Quoted 1 line

That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

If it's the same driver with different constants it should really actually be the same driver - I did ask this when the drivers were originally merged...

Quoted 3 lines · Julian Calaby

Quoted 1 line · Michal Suchanek

Quoted 1 line

Also, should the changes for the drivers be in two separate patches also?

Quoted 3 lines

Quoted 1 line

That's basically the same driver with different constants so I guess not.

Fair enough, I withdraw my comment then.

If it's the same driver with different constants it should really actually be the same driver - I did ask this when the drivers were originally merged...

The trasfer timeout is fixed at 1000 ms. Reading a 4Mbyte flash over 1MHz SPI bus takes way longer than that. Calculate the timeout from the actual time the transfer is supposed to take and multiply by 2 for good measure.

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

---

v2:
- fix build error
- use unsigned instead of int in max_t

When testing SPI without DMA I noticed that filling the FIFO on the spi controller causes timeout.

Always leave room for one byte in the FIFO.

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

---
v2:
use EMSGSIZE instead of EINVAL

---
 drivers/spi/spi-sun4i.c | 8 ++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/spi/spi-sun4i.c +0 -0

index fe63bbd..04f1b77 100644

The sun4i spi hardware can trasfer at most 63 bytes of data without DMA support so report the limitation. Same on sun6i.

Quoted 2 lines · Michal Suchanek

The sun4i spi hardware can trasfer at most 63 bytes of data without DMA support so report the limitation. Same on sun6i.

Is it? Is there timeouts on the A31 and later SoCs?

Hello,

Quoted 6 lines · Maxime Ripard

Quoted 2 lines · Michal Suchanek

The sun4i spi hardware can trasfer at most 63 bytes of data without DMA support so report the limitation. Same on sun6i.

Is it? Is there timeouts on the A31 and later SoCs?

I have no sun6i hardware to test with.

This is an advisory limit you can query and it better be smaller rather than unreliable. The hard limit in the driver is still SUN6I_FIFO_DEPTH (which is 128 bytes on sun6i).

You can test his without any actual SPI device. Unused pins you can multiplex as SPI suffice.

The maximum speed of SPI master is used when maximum speed of SPI slave is not specified. Also the __spi_validate function should check that transfer speeds do not exceed the master limits.

The user manual for A10 and A31 specifies maximum speed of the SPI clock as 100MHz and minimum as 3kHz.

Setting the SPI clock to out-of-spec values can lock up the SoC.

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

---
 drivers/spi/spi-sun4i.c | 2 ++
 drivers/spi/spi-sun6i.c | 2 ++
 2 files changed, 4 insertions(+)

drivers/spi/spi-sun4i.c +0 -0

index bf52b09..e1a75dd6 100644

- fallback to previous behaviour when DMA initialization fails

+ this has the problem that when the driver happens to load before the dma driver it will not use dma - can be addressed with a module parameter

Quoted 4 lines · Michal Suchanek

- fallback to previous behaviour when DMA initialization fails

+ this has the problem that when the driver happens to load before the dma driver it will not use dma - can be addressed with a module parameter

No, you should pay attention to the error you are getting and let probe deferral happen if that's the error you get.

Quoted 5 lines · Mark Brown

Quoted 4 lines · Michal Suchanek

- fallback to previous behaviour when DMA initialization fails

+ this has the problem that when the driver happens to load before the dma driver it will not use dma - can be addressed with a module parameter

Quoted 2 lines

No, you should pay attention to the error you are getting and let probe deferral happen if that's the error you get.

Unfortunately DMA is an optional feature.

There's no way to distinguish between -EPROBE_DEFER due to the SPI master driver being probed before the DMA engine driver, and -EPROBE_DEFER due to support for the DMA engine not having been compiled in.

Quoted 7 lines · Geert Uytterhoeven

Quoted 5 lines · Mark Brown

Quoted 4 lines · Michal Suchanek

- fallback to previous behaviour when DMA initialization fails

+ this has the problem that when the driver happens to load before the dma driver it will not use dma - can be addressed with a module parameter

Quoted 8 lines

Quoted 2 lines

No, you should pay attention to the error you are getting and let probe deferral happen if that's the error you get.

Unfortunately DMA is an optional feature.

There's no way to distinguish between -EPROBE_DEFER due to the SPI master driver being probed before the DMA engine driver, and -EPROBE_DEFER due to support for the DMA engine not having been compiled in.

I really don't think it's worth caring too much about cases where the DMA driver hasn't been compiled in, it's not like SPI is the only thing that's going to be using it. I really think it's better to defer the problem - not getting DMA (or worse, only getting DMA on some boots) is not great and if people are optimising on that level my feeling is that they're probably going to be OK with customizing DT to match. Ideally we would have something a bit nicer than deferred probe but right now this seems the more helpful option.

Quoted 2 lines · Mark Brown

I really don't think it's worth caring too much about cases where the DMA driver hasn't been compiled in, it's not like SPI is the only thing

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

It's possible to add a parameter like require_dma which could be used to load the driver without dma if unset. If it was set by default then driver ordering is not important so long as dma driver is loaded eventually. Also an informative print that such parameter exists when probing the driver is deferred would be helpful. It would probably create quite a bit of log spam, however. The driver can be deferred several times during boot.

Quoted 7 lines · Michal Suchanek

Quoted 2 lines · Mark Brown

I really don't think it's worth caring too much about cases where the DMA driver hasn't been compiled in, it's not like SPI is the only thing

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

Why? I really can't see any sensible use case for this that doesn't have a better solution available.

Quoted 8 lines

It's possible to add a parameter like require_dma which could be used to load the driver without dma if unset. If it was set by default then driver ordering is not important so long as dma driver is loaded eventually. Also an informative print that such parameter exists when probing the driver is deferred would be helpful. It would probably create quite a bit of log spam, however. The driver can be deferred several times during boot.

That seems fairly hacky, if we were going to do anything like that it should be the other way around so that we default to trying to use resources and even then it seems like something that should be handled at a framework level rather than having random options in individual drivers to ignore things. Having things behave inconsistently between different drivers is going to lead to a worse user experience and if this is a good idea for one driver it seems like it'd be a good idea for all of them.

Quoted 4 lines · Mark Brown

Quoted 2 lines · Michal Suchanek

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

Quoted 2 lines

Why? I really can't see any sensible use case for this that doesn't have a better solution available.

Of course, the solution is to compile in the DMA driver.

It's been argued that some drivers which use only short transfers will just work.

Quoted 8 lines · Michal Suchanek

Quoted 7 lines · Mark Brown

Quoted 2 lines · Michal Suchanek

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

Why? I really can't see any sensible use case for this that doesn't have a better solution available.

Quoted 4 lines

Of course, the solution is to compile in the DMA driver.

It's been argued that some drivers which use only short transfers will just work.

With nothing else in the system that needs DMA? It's making the performance of the system less reliable for the benefit of a very narrow use case.

Quoted 10 lines · Mark Brown

Quoted 8 lines · Michal Suchanek

Quoted 7 lines · Mark Brown

Quoted 2 lines · Michal Suchanek

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

Why? I really can't see any sensible use case for this that doesn't have a better solution available.

Quoted 8 lines

Quoted 4 lines

Of course, the solution is to compile in the DMA driver.

It's been argued that some drivers which use only short transfers will just work.

With nothing else in the system that needs DMA? It's making the performance of the system less reliable for the benefit of a very narrow use case.

Some of the platform devices have dedicated DMA *controller* built into the device IP so the DMA engine really is optional on many sunxi devices. Besides SPI you definitely need the DMA engine for audio. You probably don't need it for storage and graphics. I don't have any idea if it's used for USB and Ethernet.

Quoted 7 lines · Michal Suchanek

Quoted 2 lines · Mark Brown

I really don't think it's worth caring too much about cases where the DMA driver hasn't been compiled in, it's not like SPI is the only thing

It's what the driver did to start with and it was requested to fall back to non-DMA in the case DMA is not available.

Why? I really can't see any sensible use case for this that doesn't have a better solution available.

Hi,

Quoted 11 lines · Mark Brown

Quoted 9 lines · Michal Suchanek

Quoted 5 lines · Mark Brown

I really don't think it's worth caring too much about cases where the
DMA driver hasn't been compiled in, it's not like SPI is the only thing

Quoted 6 lines

Quoted 3 lines

It's what the driver did to start with and it was requested to fall
back to non-DMA in the case DMA is not available.

Why???I really can't see any sensible use case for this that doesn't
have a better solution available.

SPI works just fine without DMA, which might just be considered an (optional) optimisation.

We've been using it without DMA for years now, and it was working just
fine, and it will work even better with the other patches in this serie. There's no reason to add a hard dependency on something that we
don't really need.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

Quoted 23 lines · Maxime Ripard

Hi,

Quoted 11 lines · Mark Brown

Quoted 9 lines · Michal Suchanek

Quoted 5 lines · Mark Brown

I really don't think it's worth caring too much about cases where the
DMA driver hasn't been compiled in, it's not like SPI is the only thing

Quoted 6 lines

Quoted 3 lines

It's what the driver did to start with and it was requested to fall
back to non-DMA in the case DMA is not available.

Why? I really can't see any sensible use case for this that doesn't
have a better solution available.

SPI works just fine without DMA, which might just be considered an (optional) optimisation.

Quoted 7 lines

We've been using it without DMA for years now, and it was working just
fine, and it will work even better with the other patches in this serie. There's no reason to add a hard dependency on something that we
don't really need.

Actually it non-DMA case works fine if you don't need SPI transfers larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

Quoted 7 lines · Priit Laes

On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:

Actually it non-DMA case works fine if you don't need SPI transfers larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

And the code added in that patch will never run unless you

1) use long spi transfers
2) compile in/load SPI without DMA support

There is no reason for doing 2) since we have do DMA support for sunxi.

So that's another code path that needs maintenance and testing and likely will not get it.

Quoted 8 lines · Michal Suchanek

Quoted 7 lines · Priit Laes

On Wed, 2016-06-01 at 20:00 +0200, Maxime Ripard wrote:

Actually it non-DMA case works fine if you don't need SPI transfers larger than SUN4I_FIFO_DEPTH - 1, which is 63 bytes.

This was addressed by this patch, but was never applied:
http://permalink.gmane.org/gmane.linux.kernel.spi.devel/18950

Quoted 6 lines

And the code added in that patch will never run unless you

1) use long spi transfers
2) compile in/load SPI without DMA support

There is no reason for doing 2) since we have do DMA support for sunxi.

Well, presumably such code exists and is being worked on?

Quoted 3 lines

So that's another code path that needs maintenance and testing and likely will not get it.

Oh, come on. You might not want to use it yourself but the chances are that someone will want to use it just like the situation with all the other SPI drivers. It's a perfectly reasonable and sensible feature to support upstream.

I really do not understand why there is such a strong desire to have these devices be a special snowflake here, the worst that's likely to happen here is that you're going to end up having to either remove the DMA controller from the DT or load the driver for it neither of which seem like the end of the world.

Quoted 6 lines · Mark Brown

Quoted 4 lines · Michal Suchanek

And the code added in that patch will never run unless you

1) use long spi transfers
2) compile in/load SPI without DMA support

Quoted 3 lines

Quoted 1 line

There is no reason for doing 2) since we have do DMA support for sunxi.

Well, presumably such code exists and is being worked on?

Which code are you referring to?

This is a reply to patch which adds DMA support to the SPI driver so that it can work for arbitrarily long transfers.

Quoted 4 lines

Oh, come on. You might not want to use it yourself but the chances are that someone will want to use it just like the situation with all the other SPI drivers. It's a perfectly reasonable and sensible feature to support upstream.

Is it?

Once we have *one* driver that works for arbitrarily long transfers and it works out of the box with the board defconfig 99% of people who will use the SPI driver for anything will use this driver. Any other variant will go untested.

And for the driver to also work without DMA you have to *tell* it to probe without DMA because it cannot know you are not going to load a DMA driver later.

Quoted 5 lines

I really do not understand why there is such a strong desire to have these devices be a special snowflake here, the worst that's likely to happen here is that you're going to end up having to either remove the DMA controller from the DT or load the driver for it neither of which seem like the end of the world.

Why would you do that?