I doubt patches 3 and 4 are acceptable, but I'd still like to get
comments and/or alternative suggestions for making large transfers
faster.
The patches have been tested on an MPC8309 with a Cypress S25FL032P
spi-nor slave, and make various operations between 50% and 73%
faster.
We have not observed any problems, but to completely rule out the
possibility of "glitches on SPI CLK" mentioned in patch 3 would of
course require testing on a much wider set of hardware combinations.
Rasmus Villemoes (4):
spi: spi-fsl-spi: remove always-true conditional in fsl_spi_do_one_msg
spi: spi-fsl-spi: relax message sanity checking a little
spi: spi-fsl-spi: allow changing bits_per_word while CS is still
active
spi: spi-fsl-spi: automatically adapt bits-per-word in cpu mode
drivers/spi/spi-fsl-spi.c | 41 +++++++++++++++++++++++++++------------
1 file changed, 29 insertions(+), 12 deletions(-)
--
2.20.1
The comment says that we should not allow changes (to
bits_per_word/speed_hz) while CS is active, and indeed the code below
does fsl_spi_setup_transfer() when the ->cs_change of the previous
spi_transfer was set (and for the very first transfer).
So the sanity checking is a bit too strict - we can change it to
follow the same logic as is used by the actual transfer loop.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/spi/spi-fsl-spi.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index 6d114daa178a..481b075689b5 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -370,13 +370,15 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
int status;
/* Don't allow changes if CS is active */
- first = list_first_entry(&m->transfers, struct spi_transfer,
- transfer_list);
+ cs_change = 1;
list_for_each_entry(t, &m->transfers, transfer_list) {
+ if (cs_change)
+ first = t;
+ cs_change = t->cs_change;
if ((first->bits_per_word != t->bits_per_word) ||
(first->speed_hz != t->speed_hz)) {
dev_err(&spi->dev,
- "bits_per_word/speed_hz should be same for the same SPI transfer\n");
+ "bits_per_word/speed_hz cannot change while CS is active\n");
return -EINVAL;
}
}
--
2.20.1
Commit c9bfcb315104 (spi_mpc83xx: much improved driver) introduced
logic to ensure bits_per_word and speed_hz stay the same for a series
of spi_transfers with CS active, arguing that
The current driver may cause glitches on SPI CLK line since one
must disable the SPI controller before changing any HW settings.
This sounds quite reasonable. So this is a quite naive attempt at
relaxing this sanity checking to only ensure that speed_hz is
constant - in the faint hope that if we do not causes changes to the
clock-related fields of the SPMODE register (DIV16 and PM), those
glitches won't appear.
The purpose of this change is to allow automatically optimizing large
transfers to use 32 bits-per-word; taking one interrupt for every byte
is extremely slow.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/spi/spi-fsl-spi.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index 481b075689b5..e2b341943796 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -367,7 +367,7 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
struct spi_transfer *t, *first;
unsigned int cs_change;
const int nsecs = 50;
- int status;
+ int status, last_bpw;
/* Don't allow changes if CS is active */
cs_change = 1;
@@ -375,21 +375,22 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
if (cs_change)
first = t;
cs_change = t->cs_change;
- if ((first->bits_per_word != t->bits_per_word) ||
- (first->speed_hz != t->speed_hz)) {
+ if (first->speed_hz != t->speed_hz) {
dev_err(&spi->dev,
- "bits_per_word/speed_hz cannot change while CS is active\n");
+ "speed_hz cannot change while CS is active\n");
return -EINVAL;
}
}
+ last_bpw = -1;
cs_change = 1;
status = -EINVAL;
list_for_each_entry(t, &m->transfers, transfer_list) {
- if (cs_change)
+ if (cs_change || last_bpw != t->bits_per_word)
status = fsl_spi_setup_transfer(spi, t);
if (status < 0)
break;
+ last_bpw = t->bits_per_word;
if (cs_change) {
fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
--
2.20.1
Taking one interrupt for every byte is rather slow. Since the
controller is perfectly capable of transmitting 32 bits at a time,
change t->bits_per-word to 32 when the length is divisible by 4 and
large enough that the reduced number of interrupts easily compensates
for the one or two extra fsl_spi_setup_transfer() calls this causes.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
drivers/spi/spi-fsl-spi.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index e2b341943796..b36ac6aa3b1f 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -363,12 +363,28 @@ static int fsl_spi_bufs(struct spi_device *spi, struct spi_transfer *t,
static int fsl_spi_do_one_msg(struct spi_master *master,
struct spi_message *m)
{
+ struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(master);
struct spi_device *spi = m->spi;
struct spi_transfer *t, *first;
unsigned int cs_change;
const int nsecs = 50;
int status, last_bpw;
+ /*
+ * In CPU mode, optimize large byte transfers to use larger
+ * bits_per_word values to reduce number of interrupts taken.
+ */
+ if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
+ list_for_each_entry(t, &m->transfers, transfer_list) {
+ if (t->len < 256 || t->bits_per_word != 8)
+ continue;
+ if ((t->len & 3) == 0)
+ t->bits_per_word = 32;
+ else if ((t->len & 1) == 0)
+ t->bits_per_word = 16;
+ }
+ }
+
/* Don't allow changes if CS is active */
cs_change = 1;
list_for_each_entry(t, &m->transfers, transfer_list) {
--
2.20.1
__spi_validate() in the generic SPI code sets ->speed_hz and
->bits_per_word to non-zero values, so this condition is always true.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
This of course relies on no spi_message reaching
->transfer_one_message without having been through __spi_validate. I
believe that's ok.
drivers/spi/spi-fsl-spi.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index 3d7b50c65f36..6d114daa178a 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -384,12 +384,10 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
cs_change = 1;
status = -EINVAL;
list_for_each_entry(t, &m->transfers, transfer_list) {
- if (t->bits_per_word || t->speed_hz) {
- if (cs_change)
- status = fsl_spi_setup_transfer(spi, t);
- if (status < 0)
- break;
- }
+ if (cs_change)
+ status = fsl_spi_setup_transfer(spi, t);
+ if (status < 0)
+ break;
if (cs_change) {
fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
--
2.20.1
On Wed, Mar 27, 2019 at 02:30:48PM +0000, Rasmus Villemoes wrote:
> I doubt patches 3 and 4 are acceptable, but I'd still like to get
> comments and/or alternative suggestions for making large transfers
> faster.
I see no problem with this from a framework point of view FWIW, it's
going to be a question of if there's any glitches like you say. I'm not
sure how we can get wider testing/review unless the patches actually get
merged though... I'll leave them for a bit longer but unless someone
sees a problem I'll probably go ahead and apply them.
The patch
spi: spi-fsl-spi: relax message sanity checking a little
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 17ecffa289489e8442306bbc62ebb964e235cdad Mon Sep 17 00:00:00 2001
From: Rasmus Villemoes <[email protected]>
Date: Wed, 27 Mar 2019 14:30:51 +0000
Subject: [PATCH] spi: spi-fsl-spi: relax message sanity checking a little
The comment says that we should not allow changes (to
bits_per_word/speed_hz) while CS is active, and indeed the code below
does fsl_spi_setup_transfer() when the ->cs_change of the previous
spi_transfer was set (and for the very first transfer).
So the sanity checking is a bit too strict - we can change it to
follow the same logic as is used by the actual transfer loop.
Signed-off-by: Rasmus Villemoes <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-spi.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index 6d114daa178a..481b075689b5 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -370,13 +370,15 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
int status;
/* Don't allow changes if CS is active */
- first = list_first_entry(&m->transfers, struct spi_transfer,
- transfer_list);
+ cs_change = 1;
list_for_each_entry(t, &m->transfers, transfer_list) {
+ if (cs_change)
+ first = t;
+ cs_change = t->cs_change;
if ((first->bits_per_word != t->bits_per_word) ||
(first->speed_hz != t->speed_hz)) {
dev_err(&spi->dev,
- "bits_per_word/speed_hz should be same for the same SPI transfer\n");
+ "bits_per_word/speed_hz cannot change while CS is active\n");
return -EINVAL;
}
}
--
2.20.1
The patch
spi: spi-fsl-spi: remove always-true conditional in fsl_spi_do_one_msg
has been applied to the spi tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
From 24c363623361b430fb79459ca922e816e6f48603 Mon Sep 17 00:00:00 2001
From: Rasmus Villemoes <[email protected]>
Date: Wed, 27 Mar 2019 14:30:50 +0000
Subject: [PATCH] spi: spi-fsl-spi: remove always-true conditional in
fsl_spi_do_one_msg
__spi_validate() in the generic SPI code sets ->speed_hz and
->bits_per_word to non-zero values, so this condition is always true.
Signed-off-by: Rasmus Villemoes <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
drivers/spi/spi-fsl-spi.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index 3d7b50c65f36..6d114daa178a 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -384,12 +384,10 @@ static int fsl_spi_do_one_msg(struct spi_master *master,
cs_change = 1;
status = -EINVAL;
list_for_each_entry(t, &m->transfers, transfer_list) {
- if (t->bits_per_word || t->speed_hz) {
- if (cs_change)
- status = fsl_spi_setup_transfer(spi, t);
- if (status < 0)
- break;
- }
+ if (cs_change)
+ status = fsl_spi_setup_transfer(spi, t);
+ if (status < 0)
+ break;
if (cs_change) {
fsl_spi_chipselect(spi, BITBANG_CS_ACTIVE);
--
2.20.1
On 01/04/2019 09.34, Mark Brown wrote:
> On Wed, Mar 27, 2019 at 02:30:48PM +0000, Rasmus Villemoes wrote:
>> I doubt patches 3 and 4 are acceptable, but I'd still like to get
>> comments and/or alternative suggestions for making large transfers
>> faster.
>
> I see no problem with this from a framework point of view FWIW, it's
> going to be a question of if there's any glitches like you say. I'm not
> sure how we can get wider testing/review unless the patches actually get
> merged though... I'll leave them for a bit longer but unless someone
> sees a problem I'll probably go ahead and apply them.
>
Thanks! There's one other option I can think of: don't do the interrupts
at all, but just busy-wait for the completion of each word transfer (in
a cpu_relax() loop). That could be guarded by something like
1000000*bits_per_word < hz (roughly, the word transfer takes less than 1
us). At least on -rt, having the interrupt thread scheduled in and out
again easily takes more than 1us of cpu time, and AFAIU we'd still be
preemptible throughout - and/or one can throw in a cond_resched() every
nnn words. But this might be a bit -rt specific, and the 1us threshold
is rather arbitrary.
Rasmus
On Tue, Apr 02, 2019 at 08:43:51AM +0000, Rasmus Villemoes wrote:
> Thanks! There's one other option I can think of: don't do the interrupts
> at all, but just busy-wait for the completion of each word transfer (in
> a cpu_relax() loop). That could be guarded by something like
> 1000000*bits_per_word < hz (roughly, the word transfer takes less than 1
> us). At least on -rt, having the interrupt thread scheduled in and out
> again easily takes more than 1us of cpu time, and AFAIU we'd still be
> preemptible throughout - and/or one can throw in a cond_resched() every
> nnn words. But this might be a bit -rt specific, and the 1us threshold
> is rather arbitrary.
Yeah, that's definitely worth exploring as a mitigation but obviously
with things like flash I/O that gets a bit rude. Hopefully what's there
at the minute turns out to be robust enough.