spi/tegra114: Correct support for cs_change

PATCH RESEND Superseded Rhyland Klein 5 participants 10 messages Sep 18, 2013 – Sep 24, 2013 1 message not in the archive

Subsystems (3)

SPI Tegra architecture support Tegra SPI driver

lore

Share & tools

lore

Rhyland Klein

author

2013-09-18 18:17 UTC #0 lore raw

PATCH RESEND 1/1 spi/tegra114: Correct support for cs_change

The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.

This patch modifies the logic so that the cs state will be toggled after every individual transfer or NOT toggled at the end of the last transfer if cs_change is set in that transfer struct.

Also, this builds in logic so that if a different device tries to start a transfer while CS is active from a different device, it will abort the previous transfer and start a new one for the new device.

This work was based on the spi-atmel driver.

Signed-off-by Rhyland Klein <[email protected]>

---

RESEND WITH CORRECT PATCH ANNOTATION IN SUBJECT

 drivers/spi/spi-tegra114.c |   59 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 52 insertions(+), 7 deletions(-)

drivers/spi/spi-tegra114.c +52 -7

index 145dd43..a9973de 100644
@@ -182,6 +182,7 @@ struct tegra_spi_data {
 	u32					cur_speed;
 
 	struct spi_device			*cur_spi;
+	struct spi_device			*cs_control;
 	unsigned				cur_pos;
 	unsigned				cur_len;
 	unsigned				words_per_32bit;

@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;
 
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+			tspi->cs_control = NULL;
+		} else
+			tegra_spi_writel(tspi, command1, SPI_COMMAND1);
 
 		command1 |= SPI_CS_SW_HW;
 		if (spi->mode & SPI_CS_HIGH)

@@ -732,6 +738,9 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		command1 |= SPI_BIT_LENGTH(bits_per_word - 1);
 	}
 
+	if (t->len == 0 && !t->tx_buf && !t->rx_buf)
+		return 0;
+
 	if (tspi->is_packed)
 		command1 |= SPI_PACKED;

@@ -803,6 +812,17 @@ static int tegra_spi_setup(struct spi_device *spi)
 	return 0;
 }
 
+static void tegra_spi_transfer_delay(int delay)
+{
+	if (!delay)
+		return;
+
+	if (delay >= 1000)
+		mdelay(delay / 1000);
+
+	udelay(delay % 1000);
+}
+
 static int tegra_spi_transfer_one_message(struct spi_master *master,
 			struct spi_message *msg)
 {

@@ -812,6 +832,7 @@ static int tegra_spi_transfer_one_message(struct spi_master *master,
 	struct spi_transfer *xfer;
 	struct spi_device *spi = msg->spi;
 	int ret;
+	bool skip = false;
 
 	msg->status = 0;
 	msg->actual_length = 0;

@@ -819,13 +840,20 @@ static int tegra_spi_transfer_one_message(struct spi_master *master,
 	single_xfer = list_is_singular(&msg->transfers);
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
 		INIT_COMPLETION(tspi->xfer_completion);
+
 		ret = tegra_spi_start_transfer_one(spi, xfer,
 					is_first_msg, single_xfer);
 		if (ret < 0) {
 			dev_err(tspi->dev,
 				"spi can not start transfer, err %d\n", ret);
-			goto exit;
+			goto complete_xfer;
+		}
+
+		if (xfer->len == 0 && !xfer->tx_buf && !xfer->rx_buf) {
+			skip = true;
+			goto complete_xfer;
 		}
+
 		is_first_msg = false;
 		ret = wait_for_completion_timeout(&tspi->xfer_completion,
 						SPI_DMA_TIMEOUT);

@@ -833,24 +861,41 @@ static int tegra_spi_transfer_one_message(struct spi_master *master,
 			dev_err(tspi->dev,
 				"spi trasfer timeout, err %d\n", ret);
 			ret = -EIO;
-			goto exit;
+			goto complete_xfer;
 		}
 
 		if (tspi->tx_status ||  tspi->rx_status) {
 			dev_err(tspi->dev, "Error in Transfer\n");
 			ret = -EIO;
-			goto exit;
+			goto complete_xfer;
 		}
 		msg->actual_length += xfer->len;
-		if (xfer->cs_change && xfer->delay_usecs) {
+
+complete_xfer:
+		if (ret < 0 || skip) {
 			tegra_spi_writel(tspi, tspi->def_command1_reg,
 					SPI_COMMAND1);
-			udelay(xfer->delay_usecs);
+			tegra_spi_transfer_delay(xfer->delay_usecs);
+			goto exit;
+		} else if (msg->transfers.prev == &xfer->transfer_list) {
+			/* This is the last transfer in message */
+			if (xfer->cs_change)
+				tspi->cs_control = spi;
+			else {
+				tegra_spi_writel(tspi, tspi->def_command1_reg,
+						SPI_COMMAND1);
+				tegra_spi_transfer_delay(xfer->delay_usecs);
+			}
+		} else if (xfer->cs_change) {
+			tegra_spi_writel(tspi, tspi->def_command1_reg,
+					SPI_COMMAND1);
+			tegra_spi_transfer_delay(xfer->delay_usecs);
 		}
+
 	}
 	ret = 0;
 exit:
-	tegra_spi_writel(tspi, tspi->def_command1_reg, SPI_COMMAND1);
+
 	msg->status = ret;
 	spi_finalize_current_message(master);
 	return ret;

Signature · 1 line

1.7.9.5

Stephen Warren

2013-09-23 18:51 UTC #1 lore raw

Quoted 3 lines · Rhyland Klein

The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.

I don't really follow this patch description well. It may help if you fully spelled out the definition of normal behaviour, what the driver was doing, and what it should be doing (which is presumable what it does do after this patch).

Quoted 7 lines

This patch modifies the logic so that the cs state will be toggled after every individual transfer or NOT toggled at the end of the last transfer if cs_change is set in that transfer struct.
Also, this builds in logic so that if a different device tries to start a transfer while CS is active from a different device, it will abort the previous transfer and start a new one for the new device.

What user-visible impact does this patch have. Does it solve a bug, or improve performance, or ...? In other words, how would I test this patch, other that testing for regressions in SPI functionality that I know already works.

BTW, I don't think you're using get_maintainer.pl, since Mark Brown is the SPI maintainer now, not Grant Likely.

Quoted 13 lines

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

index 145dd43..a9973de 100644
@@ -182,6 +182,7 @@ struct tegra_spi_data {
 	u32					cur_speed;
 
 	struct spi_device			*cur_spi;
+	struct spi_device			*cs_control;
 	unsigned				cur_pos;
 	unsigned				cur_len;
 	unsigned				words_per_32bit;

drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723

Quoted 7 lines

drivers/spi/spi-tegra114.c +3 -1

@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;
 
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);

Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?

drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 742

Quoted 12 lines

drivers/spi/spi-tegra114.c +5 -0

+			tspi->cs_control = NULL;
+		} else
+			tegra_spi_writel(tspi, command1, SPI_COMMAND1);
 
 		command1 |= SPI_CS_SW_HW;
 		if (spi->mode & SPI_CS_HIGH)

@@ -732,6 +738,9 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		command1 |= SPI_BIT_LENGTH(bits_per_word - 1);
 	}
 
+	if (t->len == 0 && !t->tx_buf && !t->rx_buf)
+		return 0;

What if !t->len, but t->tx_buf || t->rx_buf ?

This code is also duplicated in tegra_spi_transfer_one_message() after this patch. Instead, perhaps the first N lines of tegra_spi_start_transfer_one() should be split out into e.g. tegra_spi_setup_transfer_one(), such that tegra_spi_transfer_one_message() can always call it, plus have tegra_spi_transfer_one_message() only call tegra_spi_start_transfer_one() if (t->len != 0).

Rhyland Klein

author

2013-09-23 19:48 UTC #2 lore raw

Quoted 9 lines · Stephen Warren

Quoted 3 lines · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.
I don't really follow this patch description well. It may help if you fully spelled out the definition of normal behaviour, what the driver was doing, and what it should be doing (which is presumable what it does do after this patch).

Yah, I'll explain the cs_change logic a little clearer in the next version, but for now:

cs_change is meant to invert the normal behavior of the CS line during a SPI transfer. Normally, the CS line stays asserted from the beginning of the first transfer until the last is completed within the same spi_message, and then it is deasserted. There can be any number of spi_transfers within a spi_message. When cs_change is asserted for a spi_transfer that is not the last one in the message, this means that after that transfer is completed, the CS line should be deasserted (where normally it would stay asserted). Then when the next transfer starts, it will assert CS again. If the last spi_transfer in a spi_message has cs_change set, then it means that instead of deasserted CS at the end of the transaction (spi_message) then it leaves CS asserted.

Quoted 5 lines

Quoted 5 lines
This patch modifies the logic so that the cs state will be toggled after every individual transfer or NOT toggled at the end of the last transfer if cs_change is set in that transfer struct.

Quoted 8 lines

Quoted 3 lines
Also, this builds in logic so that if a different device tries to start a transfer while CS is active from a different device, it will abort the previous transfer and start a new one for the new device.
What user-visible impact does this patch have. Does it solve a bug, or improve performance, or ...? In other words, how would I test this patch, other that testing for regressions in SPI functionality that I know already works.

This allows a spi client to send a write, and then any number of reads as a single spi transaction, but separated into separate spi_messages. I tested this for ChromeOS where our platform has an embedded controller which communicates over SPI. It has some quirks, namely when we read back data, we maybe need to do multiple reads before we start getting valid data. This means we need to be able to control when the CS line is deasserted which would be after we know we finally received all the data we were expecting.

I don't know of any other hardware currently that is good for exercising this logic on our reference boards.

Quoted 3 lines

BTW, I don't think you're using get_maintainer.pl, since Mark Brown is the SPI maintainer now, not Grant Likely.

I could have sworn I ran it, but I didn't see Mark come up, I'll definitely add him for the next revision.

Quoted 17 lines

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

index 145dd43..a9973de 100644
@@ -182,6 +182,7 @@ struct tegra_spi_data {
 	u32					cur_speed;
 
 	struct spi_device			*cur_spi;
+	struct spi_device			*cs_control;
 	unsigned				cur_pos;
 	unsigned				cur_len;
 	unsigned				words_per_32bit;

@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;

Quoted 8 lines

drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723
Quoted 4 lines
drivers/spi/spi-tegra114.c +3 -1
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?

On our hardware (as far as I've seen), the CS line is normally low. We need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.

Quoted 16 lines

drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 742

Quoted 13 lines

drivers/spi/spi-tegra114.c +5 -0

+			tspi->cs_control = NULL;
+		} else
+			tegra_spi_writel(tspi, command1, SPI_COMMAND1);
 
 		command1 |= SPI_CS_SW_HW;
 		if (spi->mode & SPI_CS_HIGH)

@@ -732,6 +738,9 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		command1 |= SPI_BIT_LENGTH(bits_per_word - 1);
 	}
 
+	if (t->len == 0 && !t->tx_buf && !t->rx_buf)
+		return 0;

What if !t->len, but t->tx_buf || t->rx_buf ?

Quoted 7 lines

This code is also duplicated in tegra_spi_transfer_one_message() after this patch. Instead, perhaps the first N lines of tegra_spi_start_transfer_one() should be split out into e.g. tegra_spi_setup_transfer_one(), such that tegra_spi_transfer_one_message() can always call it, plus have tegra_spi_transfer_one_message() only call tegra_spi_start_transfer_one() if (t->len != 0).

I'll look into this some more. The main reason I have this here is because tegra_spi_start_transfer_one() contains the logic for starting a transaction (which when this type of spi_transfer is set, can be used just to toggle CS and end a previous transaction that had cs_change set). Breaking apart tegra_spi_start_transfer_one() might very well be much nicer. I'll see how it looks.

Thanks,
-rhyland

Signature · 1 line

nvpublic

Stephen Warren

2013-09-23 19:58 UTC #3 lore raw

Quoted 5 lines · Rhyland Klein

Quoted 4 lines · Stephen Warren
Quoted 3 lines · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.

...

Quoted 1 line

Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0

...

Quoted 13 lines

Quoted 4 lines
Quoted 4 lines
drivers/spi/spi-tegra114.c +0 -0
@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;
 
Quoted 8 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723
Quoted 4 lines
drivers/spi/spi-tegra114.c +3 -1
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?

Quoted 1 line

On our hardware (as far as I've seen), the CS line is normally low. We

I assume you meant "normally *active* low", not "normally low"?

Quoted 3 lines

need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.

That sounds like exactly the glitch I was talking about.

Assuming CS isn't held constantly asserted (low), isn't CS de-asserted (rises) at the end of transaction n-1, and re-asserted (falls) at the start of transaction n? If so, I'm not sure why the setup for transaction n needs to both de-assert and then re-assert it? It seems like cs_control should be handled at the end of a transaction, not at the start of the next one.

Rhyland Klein

author

2013-09-23 21:01 UTC #4 lore raw

Quoted 22 lines · Stephen Warren

Quoted 5 lines · Rhyland Klein
Quoted 4 lines · Stephen Warren
Quoted 3 lines · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0
...
Quoted 13 lines
Quoted 12 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723
Quoted 8 lines
drivers/spi/spi-tegra114.c +3 -1
@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;
 
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?

Quoted 3 lines

Quoted 1 line
On our hardware (as far as I've seen), the CS line is normally low. We
I assume you meant "normally *active* low", not "normally low"?

I mean that when I look at CS, before a transaction starts, the line is low. Then we do a write like this (default SPI_COMMAND1) you see CS rise and fall very soon after. This is purely how we generate the edge triggers (as far as I can tell on all Tegra hw, though Laxman can correct me if I am wrong).

Quoted 5 lines

Quoted 4 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.

Quoted 8 lines

That sounds like exactly the glitch I was talking about.
Assuming CS isn't held constantly asserted (low), isn't CS de-asserted (rises) at the end of transaction n-1, and re-asserted (falls) at the start of transaction n? If so, I'm not sure why the setup for transaction n needs to both de-assert and then re-assert it? It seems like cs_control should be handled at the end of a transaction, not at the start of the next one.

cs_change has to maintain state over spi_message boundries, not just between spi_transfers within a spi_message.

In this specific case, this is a safe guard.

Quoted 1 line

Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 721
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+		if (tspi->cs_control) {

This sees that the previous transfer stored its spi_device pointer, meaning it had cs_change set on the last part of the last spi_message (I.E. CS hasn't been deasserted, so the SPI transaction is still on-going).

Quoted 1 line

Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 722
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+			if (tspi->cs_control != spi)

This however finds that the device trying to send a SPI message isn't the same one that was in the middle of its transaction. This is a collision between 2 clients on 1 bus. This simply ends the previous transaction (the ongoing one) in favor of starting a new transaction.

Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.

-rhyland

Signature · 1 line

nvpublic

Stephen Warren

2013-09-23 21:14 UTC #5 lore raw

Quoted 25 lines · Rhyland Klein

Quoted 24 lines · Stephen Warren
Quoted 5 lines · Rhyland Klein
Quoted 4 lines · Stephen Warren
Quoted 3 lines · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0
...
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
drivers/spi/spi-tegra114.c +0 -0
@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
 		else if (req_mode == SPI_MODE_3)
 			command1 |= SPI_CONTROL_MODE_3;
 
Quoted 8 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723
Quoted 4 lines
drivers/spi/spi-tegra114.c +3 -1
-		tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+		if (tspi->cs_control) {
+			if (tspi->cs_control != spi)
+				tegra_spi_writel(tspi, command1, SPI_COMMAND1);
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?
On our hardware (as far as I've seen), the CS line is normally low. We

Quoted 7 lines

Quoted 1 line
I assume you meant "normally *active* low", not "normally low"?
I mean that when I look at CS, before a transaction starts, the line is low. Then we do a write like this (default SPI_COMMAND1) you see CS rise and fall very soon after. This is purely how we generate the edge triggers (as far as I can tell on all Tegra hw, though Laxman can correct me if I am wrong).

That sounds broken. Normally, shouldn't CS assert before a transaction, stay asserted during a transaction, then deassert after the transaction? It shouldn't rise and fall very quickly in between parts of the transaction.

Quoted 29 lines

Quoted 4 lines
Quoted 3 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.
Quoted 8 lines
That sounds like exactly the glitch I was talking about.
Assuming CS isn't held constantly asserted (low), isn't CS de-asserted (rises) at the end of transaction n-1, and re-asserted (falls) at the start of transaction n? If so, I'm not sure why the setup for transaction n needs to both de-assert and then re-assert it? It seems like cs_control should be handled at the end of a transaction, not at the start of the next one.
cs_change has to maintain state over spi_message boundries, not just between spi_transfers within a spi_message.
In this specific case, this is a safe guard.
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 721
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+		if (tspi->cs_control) {
This sees that the previous transfer stored its spi_device pointer, meaning it had cs_change set on the last part of the last spi_message (I.E. CS hasn't been deasserted, so the SPI transaction is still on-going).
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 722
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+			if (tspi->cs_control != spi)
This however finds that the device trying to send a SPI message isn't the same one that was in the middle of its transaction. This is a collision between 2 clients on 1 bus. This simply ends the previous transaction (the ongoing one) in favor of starting a new transaction.

Quoted 3 lines

Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.

This sounds like something the SPI core should be managing. If a driver is using the SPI bus to communicate with a device in a way that needs CS left active while outside a transaction, it shouldn't be possible for another driver to come along and communicate with another device before the first transaction is all complete. The bus should be locked. Allowing anything else could corrupt the protocol that required specific CS states outside the transaction.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Simon Glass

2013-09-23 22:23 UTC #7 lore raw

Hi,

Quoted 78 lines · Stephen Warren

Quoted 5 lines · Rhyland Klein
Quoted 4 lines · Stephen Warren
Quoted 3 lines · Rhyland Klein
Quoted 2 lines · Stephen Warren
Quoted 1 line · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change
functionality.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
It is meant to invert normal behavior, and we were only using it to
possibly
Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
delay at the end of a transfer.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0
...
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0
@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct
spi_device *spi,
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct · line 723
Quoted 7 lines
drivers/spi/spi-tegra114.c +3 -1
          else if (req_mode == SPI_MODE_3)
                  command1 |= SPI_CONTROL_MODE_3;
 
-         tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+         if (tspi->cs_control) {
+                 if (tspi->cs_control != spi)
+                         tegra_spi_writel(tspi, command1,
SPI_COMMAND1);
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately
after),
Quoted 11 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
so won't that glitch the CS lines in some cases?
On our hardware (as far as I've seen), the CS line is normally low. We
I assume you meant "normally *active* low", not "normally low"?
I mean that when I look at CS, before a transaction starts, the line is low. Then we do a write like this (default SPI_COMMAND1) you see CS rise and fall very soon after. This is purely how we generate the edge triggers (as far as I can tell on all Tegra hw, though Laxman can correct me if I am wrong).
That sounds broken. Normally, shouldn't CS assert before a transaction, stay asserted during a transaction, then deassert after the transaction? It shouldn't rise and fall very quickly in between parts of the transaction.
Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.
Quoted 8 lines
That sounds like exactly the glitch I was talking about.
Assuming CS isn't held constantly asserted (low), isn't CS de-asserted (rises) at the end of transaction n-1, and re-asserted (falls) at the start of transaction n? If so, I'm not sure why the setup for transaction n needs to both de-assert and then re-assert it? It seems like cs_control should be handled at the end of a transaction, not at the start of the next one.
cs_change has to maintain state over spi_message boundries, not just between spi_transfers within a spi_message.
Quoted 6 lines
In this specific case, this is a safe guard.
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct · line 724
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+         if (tspi->cs_control) {
This sees that the previous transfer stored its spi_device pointer, meaning it had cs_change set on the last part of the last spi_message (I.E. CS hasn't been deasserted, so the SPI transaction is still
on-going).
Quoted 10 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct · line 725
Quoted 2 lines
drivers/spi/spi-tegra114.c +1 -0
+                 if (tspi->cs_control != spi)
This however finds that the device trying to send a SPI message isn't the same one that was in the middle of its transaction. This is a collision between 2 clients on 1 bus. This simply ends the previous transaction (the ongoing one) in favor of starting a new transaction.
Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.

Quoted 8 lines

This sounds like something the SPI core should be managing. If a driver is using the SPI bus to communicate with a device in a way that needs CS left active while outside a transaction, it shouldn't be possible for another driver to come along and communicate with another device before the first transaction is all complete. The bus should be locked. Allowing anything else could corrupt the protocol that required specific CS states outside the transaction.

Perhaps the client driver should use spi_sync_locked() instead if it wants to avoid this problem? To me this driver code seems reasonable in the circumstances.

Note: at present the Chrome OS EC driver does not support sharing a SPI bus with anything else, and neither does the EC.

Regards,
Simon
------------------------------------------------------------------------------
October Webinars: Code for Performance
Free Intel webinars can help you accelerate application performance. Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most from the latest Intel processors and coprocessors. See abstracts and register >
http://pubads.g.doubleclick.net/gampad/clk?id=60133471&iu=/4140/ostg.clktrk

Simon Glass

2013-09-23 22:24 UTC #8 lore raw

In reply to Stephen Warren

[trying again]

Hi,

Quoted 71 lines · Stephen Warren

Quoted 25 lines · Rhyland Klein
Quoted 24 lines · Stephen Warren
Quoted 5 lines · Rhyland Klein
Quoted 4 lines · Stephen Warren
Quoted 3 lines · Rhyland Klein
The tegra114 driver wasn't currently handling the cs_change functionality. It is meant to invert normal behavior, and we were only using it to possibly delay at the end of a transfer.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c +0 -0
...
Quoted 14 lines
Quoted 4 lines
Quoted 4 lines
drivers/spi/spi-tegra114.c +0 -0
@@ -717,7 +718,12 @@ static int tegra_spi_start_transfer_one(struct spi_device *spi,
          else if (req_mode == SPI_MODE_3)
                  command1 |= SPI_CONTROL_MODE_3;
 
Quoted 8 lines
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 723
Quoted 4 lines
drivers/spi/spi-tegra114.c +3 -1
-         tegra_spi_writel(tspi, command1, SPI_COMMAND1);
+         if (tspi->cs_control) {
+                 if (tspi->cs_control != spi)
+                         tegra_spi_writel(tspi, command1, SPI_COMMAND1);
Do you really need a separate write call there? The value of command1 isn't fully set up there (the CS bits are all set up immediately after), so won't that glitch the CS lines in some cases?
On our hardware (as far as I've seen), the CS line is normally low. We
Quoted 7 lines
Quoted 1 line
I assume you meant "normally *active* low", not "normally low"?
I mean that when I look at CS, before a transaction starts, the line is low. Then we do a write like this (default SPI_COMMAND1) you see CS rise and fall very soon after. This is purely how we generate the edge triggers (as far as I can tell on all Tegra hw, though Laxman can correct me if I am wrong).
That sounds broken. Normally, shouldn't CS assert before a transaction, stay asserted during a transaction, then deassert after the transaction? It shouldn't rise and fall very quickly in between parts of the transaction.
Quoted 24 lines
Quoted 4 lines
Quoted 3 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.
Quoted 8 lines
That sounds like exactly the glitch I was talking about.
Assuming CS isn't held constantly asserted (low), isn't CS de-asserted (rises) at the end of transaction n-1, and re-asserted (falls) at the start of transaction n? If so, I'm not sure why the setup for transaction n needs to both de-assert and then re-assert it? It seems like cs_control should be handled at the end of a transaction, not at the start of the next one.
cs_change has to maintain state over spi_message boundries, not just between spi_transfers within a spi_message.
In this specific case, this is a safe guard.
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 721
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+         if (tspi->cs_control) {
This sees that the previous transfer stored its spi_device pointer, meaning it had cs_change set on the last part of the last spi_message (I.E. CS hasn't been deasserted, so the SPI transaction is still on-going).
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/spi/spi-tegra114.c static int tegra_spi_start_transfer_one(struct spi_device *spi, · line 722
Quoted 1 line
drivers/spi/spi-tegra114.c +1 -0
+                 if (tspi->cs_control != spi)
Quoted 8 lines
This however finds that the device trying to send a SPI message isn't the same one that was in the middle of its transaction. This is a collision between 2 clients on 1 bus. This simply ends the previous transaction (the ongoing one) in favor of starting a new transaction.
Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.

Quoted 7 lines

This sounds like something the SPI core should be managing. If a driver is using the SPI bus to communicate with a device in a way that needs CS left active while outside a transaction, it shouldn't be possible for another driver to come along and communicate with another device before the first transaction is all complete. The bus should be locked. Allowing anything else could corrupt the protocol that required specific CS states outside the transaction.

Perhaps the client driver should use spi_sync_locked() instead if it wants to avoid this problem? To me this driver code seems reasonable in the circumstances.

Note: at present the Chrome OS EC driver does not support sharing a SPI bus with anything else, and neither does the EC.

Regards,
Simon

Trent Piepho

2013-09-23 23:08 UTC #9 lore raw

In reply to Stephen Warren

Quoted 4 lines · Stephen Warren

That sounds broken. Normally, shouldn't CS assert before a transaction, stay asserted during a transaction, then deassert after the transaction? It shouldn't rise and fall very quickly in between parts of the transaction.

That is normal, where a transaction is a spi_message made up of multiple spi_transfers. The cs_change bit for a transfer will insert a de-asserted pulse after a transfer or leave CS de-asserted after the last transfer.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.

I wonder, is the real problem that the spi layer allows CS to possibly remain asserted between transactions to the same device? Normally you would expect it to be de-asserted at the end of a spi_message, but I believe the Linux spi semantics are that it may or may not actually be de-asserted at that time. It only guarantees that is will be de-asserted before a message to a different device starts.

I guess this is supposed to be an optimization. Some drivers, like gpio bit-banging, probably have a cost associated with any CS change. Usually many messages in a row are to the same device. Most devices don't care if CS pulses between messages. Thus not pulsing CS between each message is faster.

Quoted 5 lines

Quoted 4 lines
Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.

Quoted 7 lines

This sounds like something the SPI core should be managing. If a driver is using the SPI bus to communicate with a device in a way that needs CS left active while outside a transaction, it shouldn't be possible for another driver to come along and communicate with another device before the first transaction is all complete. The bus should be locked. Allowing anything else could corrupt the protocol that required specific CS states outside the transaction.

If the transaction is one message, which can be multiple transfers and multiple CS pulses, then the spi core always does it atomically. The limitation is the driver can't get the result of the transaction until the entire transaction is finished.

If a driver needs to get part of a transaction to complete the rest, e.g. read a 16-bit length from the device and then read that many bytes, it can still be done. It doesn't seem to be documented in spi-summary, but the way to do this is with spi_bus_(un)lock() and spi_(a)sync_locked() calls. The driver must lock the bus, used the _locked versions to issue spi_messages, then unlock when done. This should prevent another device on the bus from getting a messages, and thus CS pulses, in the middle of the transaction.

Rhyland Klein

author

2013-09-24 16:33 UTC #10 lore raw

Quoted 46 lines · Trent Piepho

Quoted 4 lines · Stephen Warren
That sounds broken. Normally, shouldn't CS assert before a transaction, stay asserted during a transaction, then deassert after the transaction? It shouldn't rise and fall very quickly in between parts of the transaction.
That is normal, where a transaction is a spi_message made up of multiple spi_transfers. The cs_change bit for a transfer will insert a de-asserted pulse after a transfer or leave CS de-asserted after the last transfer.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
need to generate a falling-edge to trigger the beginning of a SPI transaction. Doing this write with the default value of SPI_COMMAND1 causes a brief rise and fall of CS, giving us our falling-edge.
I wonder, is the real problem that the spi layer allows CS to possibly remain asserted between transactions to the same device? Normally you would expect it to be de-asserted at the end of a spi_message, but I believe the Linux spi semantics are that it may or may not actually be de-asserted at that time. It only guarantees that is will be de-asserted before a message to a different device starts.
I guess this is supposed to be an optimization. Some drivers, like gpio bit-banging, probably have a cost associated with any CS change. Usually many messages in a row are to the same device. Most devices don't care if CS pulses between messages. Thus not pulsing CS between each message is faster.
Quoted 5 lines
Quoted 4 lines
Otherwise, this logic allows us to skip the spi of COMMAND1 which would normally be used to create the falling edge to start a new transaction, leaving the previous one open for more transfers.
Quoted 7 lines
This sounds like something the SPI core should be managing. If a driver is using the SPI bus to communicate with a device in a way that needs CS left active while outside a transaction, it shouldn't be possible for another driver to come along and communicate with another device before the first transaction is all complete. The bus should be locked. Allowing anything else could corrupt the protocol that required specific CS states outside the transaction.
If the transaction is one message, which can be multiple transfers and multiple CS pulses, then the spi core always does it atomically. The limitation is the driver can't get the result of the transaction until the entire transaction is finished.

Quoted 8 lines

If a driver needs to get part of a transaction to complete the rest, e.g. read a 16-bit length from the device and then read that many bytes, it can still be done. It doesn't seem to be documented in spi-summary, but the way to do this is with spi_bus_(un)lock() and spi_(a)sync_locked() calls. The driver must lock the bus, used the _locked versions to issue spi_messages, then unlock when done. This should prevent another device on the bus from getting a messages, and thus CS pulses, in the middle of the transaction.

I suppose I can should reword my comment then on the code which checked:

+ if (tspi->cs_control) {
+ if (tspi->cs_control != spi)
+ tegra_spi_writel(tspi, command1, SPI_COMMAND1);

While this does do exactly what I said, ending a previous "on-going" transaction in favor of a new one, this shouldn't be expected to be the way for clients to guarantee that they have locked a bus. This is more of a way internally to the Tegra SPI driver to clear its state.

-rhyland

Signature · 1 line

nvpublic