Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On Mon, May 25, 2020 at 03:12:05PM +0200, Marc Kleine-Budde wrote: > On 5/25/20 2:57 PM, Mark Brown wrote: > > On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote: > >> On 5/25/20 1:31 PM, Mark Brown wrote: > >> The core could merge several half duplex transfers (until there's as > >> cs_change) > >> into a single full duplex transfer. > > Yes, that is what I am suggesting. > Where in the SPI stack do you see such a "merge" function? One point to > clarify > is when and where to allocate and free the memory for the contiguous full > duplex > buffers. My first thought would be about the same point as we're rewriting to handle MUST_TX and MUST_RX in map_msg() which does similar allocations and deallocations to insert dummy data for controllers that need them. > >> I think spi_write_then_read() can be extended to generate one full duplex > >> transfer instead on two half duplex ones it does a memcpy() anyways. > > This has the same problem as doing it in any other driver code - it > > causes a needless incompatibility with three wire and single duplex > > devices. > What about the note "portable code should never use this for more than 32 > bytes" > in spi_write_then_read()? The CAN driver in question may read more than 32 > bytes > of data. I think that comment is actually not valid any more - we used to use a fixed statically allocated buffer in write_then_read() but added the option to fall back onto allocating one dynamically if another user was running or the transfer was too big. signature.asc Description: PGP signature
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On 5/25/20 2:57 PM, Mark Brown wrote: > On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote: >> On 5/25/20 1:31 PM, Mark Brown wrote: > >>> This isn't something that every individual driver should be doing, such >>> rewriting should happen in the core so that everything sees the benefit. > >> The core could merge several half duplex transfers (until there's as >> cs_change) >> into a single full duplex transfer. > > Yes, that is what I am suggesting. Where in the SPI stack do you see such a "merge" function? One point to clarify is when and where to allocate and free the memory for the contiguous full duplex buffers. >> I think it's not easy to detect and reliable to split a full duplex transfer >> into half duplex ones. How can you tell, if the controller is supposed to tx >> 0x0 >> or actually receive. > > I don't understand how that could possibly work or why it would make > sense? ACK, I was just thinking loud about options. >> I think spi_write_then_read() can be extended to generate one full duplex >> transfer instead on two half duplex ones it does a memcpy() anyways. > > This has the same problem as doing it in any other driver code - it > causes a needless incompatibility with three wire and single duplex > devices. What about the note "portable code should never use this for more than 32 bytes" in spi_write_then_read()? The CAN driver in question may read more than 32 bytes of data. >> To get a feeling for the use cases, this is what I do in the regmap read >> function of a (not yet mainlined) CAN SPI driver. > > Like I say it's probably better if code like this gets pushed into the > SPI core where we've got more information about what the controller can > do and there's more win from doing the tuning since more devices and > systems can take advantage of it. ACK Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On Mon, May 25, 2020 at 02:41:31PM +0200, Marc Kleine-Budde wrote: > On 5/25/20 1:31 PM, Mark Brown wrote: > > This isn't something that every individual driver should be doing, such > > rewriting should happen in the core so that everything sees the benefit. > The core could merge several half duplex transfers (until there's as > cs_change) > into a single full duplex transfer. Yes, that is what I am suggesting. > I think it's not easy to detect and reliable to split a full duplex transfer > into half duplex ones. How can you tell, if the controller is supposed to tx > 0x0 > or actually receive. I don't understand how that could possibly work or why it would make sense? > I think spi_write_then_read() can be extended to generate one full duplex > transfer instead on two half duplex ones it does a memcpy() anyways. This has the same problem as doing it in any other driver code - it causes a needless incompatibility with three wire and single duplex devices. > To get a feeling for the use cases, this is what I do in the regmap read > function of a (not yet mainlined) CAN SPI driver. Like I say it's probably better if code like this gets pushed into the SPI core where we've got more information about what the controller can do and there's more win from doing the tuning since more devices and systems can take advantage of it. signature.asc Description: PGP signature
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On 5/25/20 1:31 PM, Mark Brown wrote:
>>> Should I be submitting this patch with logic that only does
>>> half-duplex if the spi controller doesn't support it (if
>>> (spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it
>>> acceptable to simply make the driver half-duplex like this for all
>>> cases?
>
>> Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as
>> most drivers have a considerable overhead at the end of a transfer.
>
>> Most of them wait for a transfer complete interrupt. Which might take longer
>> than the actual SPI transfer. Splitting one full duplex read-register
>> transfer
>> (which is a write followed by a read) into two half duplex transfers would
>> kill
>> performance on full duplex capable controllers.
>
> This isn't something that every individual driver should be doing, such
> rewriting should happen in the core so that everything sees the benefit.
The core could merge several half duplex transfers (until there's as cs_change)
into a single full duplex transfer.
I think it's not easy to detect and reliable to split a full duplex transfer
into half duplex ones. How can you tell, if the controller is supposed to tx 0x0
or actually receive.
I think spi_write_then_read() can be extended to generate one full duplex
transfer instead on two half duplex ones it does a memcpy() anyways.
To get a feeling for the use cases, this is what I do in the regmap read
function of a (not yet mainlined) CAN SPI driver.
> static int
> mcp25xxfd_regmap_nocrc_read(void *context,
> const void *reg, size_t reg_len,
> void *val_buf, size_t val_len)
> {
> struct spi_device *spi = context;
> struct mcp25xxfd_priv *priv = spi_get_drvdata(spi);
> struct mcp25xxfd_map_buf_nocrc *buf_rx = priv->map_buf_nocrc_rx;
> struct mcp25xxfd_map_buf_nocrc *buf_tx = priv->map_buf_nocrc_tx;
> struct spi_transfer xfer[2] = { };
> struct spi_message msg;
> int err;
>
> spi_message_init();
> spi_message_add_tail([0], );
>
> if (priv->devtype_data.quirks & MCP25XXFD_QUIRK_HALF_DUPLEX) {
> xfer[0].tx_buf = reg;
> xfer[0].len = sizeof(buf_tx->cmd);
>
> xfer[1].rx_buf = val_buf;
> xfer[1].len = val_len;
> spi_message_add_tail([1], );
> } else {
> xfer[0].tx_buf = buf_tx;
> xfer[0].rx_buf = buf_rx;
> xfer[0].len = sizeof(buf_tx->cmd) + val_len;
> memcpy(_tx->cmd, reg, sizeof(buf_tx->cmd));
> };
>
> err = spi_sync(spi, );
> if (err)
> return err;
>
> if (!(priv->devtype_data.quirks & MCP25XXFD_QUIRK_HALF_DUPLEX))
> memcpy(val_buf, buf_rx->data, val_len);
>
> return 0;
> }
The tradeoff here is two transfers vs. the the memcpy(). As CAN frames are quite
small the memcpy() is usually faster. Even on the rpi, where the driver is
optimized for small transfers.
regards
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On Mon, May 25, 2020 at 01:17:01PM +0200, Marc Kleine-Budde wrote: > On 5/21/20 10:19 PM, Tim Harvey wrote: > > > Should I be submitting this patch with logic that only does > > half-duplex if the spi controller doesn't support it (if > > (spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it > > acceptable to simply make the driver half-duplex like this for all > > cases? > Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as > most drivers have a considerable overhead at the end of a transfer. > Most of them wait for a transfer complete interrupt. Which might take longer > than the actual SPI transfer. Splitting one full duplex read-register transfer > (which is a write followed by a read) into two half duplex transfers would > kill > performance on full duplex capable controllers. This isn't something that every individual driver should be doing, such rewriting should happen in the core so that everything sees the benefit. signature.asc Description: PGP signature
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On 5/21/20 10:19 PM, Tim Harvey wrote:
> On Wed, Feb 26, 2020 at 2:19 AM Marc Kleine-Budde wrote:
> Sorry for the long delay... I'm finally getting back to this issue.
>
> I'm told by Marvell/Cavium that the OcteonTX SPI hardware does not
> support full duplex although I don't see this in any of their errata
> or reference manuals. Perhaps someone familiar with the CN81xx/CN80xx
> OcteonTX hardware from Marvell/Cavium can weigh in here as I'm not
> clear if this limitation is in all hardware that uses the
> spi-cavium-thunderx.c driver (I've added Jan to the list who authored
> the driver)
If the hardware doesn't support full duplex transfers you should add
SPI_CONTROLLER_HALF_DUPLEX to the driver.
If the hardware supports full duplex, but the driver doesn't the driver should
be fixed, or SPI_CONTROLLER_HALF_DUPLEX with the appropriate explanation.
> As you point out setting SPI_CONTROLLER_HALF_DUPLEX will cause
> spi_{sync,async,async_locked} calls to fail with -EINVAL if they have
> both a tx and rx buf, so this should be done to help catch these
> issues:
> diff --git a/drivers/spi/spi-cavium-thunderx.c
> b/drivers/spi/spi-cavium-thunderx.c
> index fd6b9ca..76fdb94 100644
> --- a/drivers/spi/spi-cavium-thunderx.c
> +++ b/drivers/spi/spi-cavium-thunderx.c
> @@ -64,6 +65,7 @@ static int thunderx_spi_probe(struct pci_dev *pdev,
> p->sys_freq = SYS_FREQ_DEFAULT;
> dev_info(dev, "Set system clock to %u\n", p->sys_freq);
>
> + master->flags = SPI_MASTER_HALF_DUPLEX;
> master->num_chipselect = 4;
> master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH |
> SPI_LSB_FIRST | SPI_3WIRE;
>
> Now, with regards to the mcp251x.c driver you were correct that I was
> missing dealing with the full-duplex call from mcp251x_hw_rx_frame()
> which indeed was causing data corruption on recieve.
>
> So the following patch to mcp251x.c properly converts mcp251x to half-duplex:
>
> diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
> index 5009ff2..016c1e5 100644
> --- a/drivers/net/can/spi/mcp251x.c
> +++ b/drivers/net/can/spi/mcp251x.c
> @@ -290,23 +290,23 @@ static u8 mcp251x_read_reg(struct spi_device *spi, u8
> reg)
> priv->spi_tx_buf[0] = INSTRUCTION_READ;
> priv->spi_tx_buf[1] = reg;
>
> - mcp251x_spi_trans(spi, 3);
> - val = priv->spi_rx_buf[2];
> + spi_write_then_read(spi, priv->spi_tx_buf, 2, , 1);
>
> return val;
> }
>
> static void mcp251x_read_2regs(struct spi_device *spi, u8 reg, u8 *v1, u8
> *v2)
> {
> + u8 val[2] = {0};
> struct mcp251x_priv *priv = spi_get_drvdata(spi);
>
> priv->spi_tx_buf[0] = INSTRUCTION_READ;
> priv->spi_tx_buf[1] = reg;
>
> - mcp251x_spi_trans(spi, 4);
> + spi_write_then_read(spi, priv->spi_tx_buf, 2, val, 2);
>
> - *v1 = priv->spi_rx_buf[2];
> - *v2 = priv->spi_rx_buf[3];
> + *v1 = val[0];
> + *v2 = val[1];
> }
>
> static void mcp251x_write_reg(struct spi_device *spi, u8 reg, u8 val)
> @@ -409,8 +409,9 @@ static void mcp251x_hw_rx_frame(struct spi_device
> *spi, u8 *buf,
> buf[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i);
> } else {
> priv->spi_tx_buf[RXBCTRL_OFF] = INSTRUCTION_READ_RXB(buf_idx);
> - mcp251x_spi_trans(spi, SPI_TRANSFER_BUF_LEN);
> - memcpy(buf, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN);
> + spi_write_then_read(spi, priv->spi_tx_buf, 1,
> priv->spi_rx_buf,
> + SPI_TRANSFER_BUF_LEN);
> + memcpy(buf + 1, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN - 1);
> }
> }
>
> I do have hardware to test with and without this patch my CN80xx board
> with an MCP25625 fails device probing (mcp251x spi0.1: MCP251x didn't
> enter in conf mode after reset) because read values are corrupt. With
> this patch my the MCP25625 works fine on the CN80xx detecting,
> sending, and receiving frames.
nice!
> Should I be submitting this patch with logic that only does
> half-duplex if the spi controller doesn't support it (if
> (spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it
> acceptable to simply make the driver half-duplex like this for all
> cases?
Please make half duplex transfers depending on SPI_CONTROLLER_HALF_DUPLEX as
most drivers have a considerable overhead at the end of a transfer.
Most of them wait for a transfer complete interrupt. Which might take longer
than the actual SPI transfer. Splitting one full duplex read-register transfer
(which is a write followed by a read) into two half duplex transfers would kill
performance on full duplex capable controllers.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 |
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On Thu, May 21, 2020 at 01:19:16PM -0700, Tim Harvey wrote: > Should I be submitting this patch with logic that only does > half-duplex if the spi controller doesn't support it (if > (spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it > acceptable to simply make the driver half-duplex like this for all > cases? It seems likely that making the transfers explicitly half duplex will perform better, especially for PIO controllers since there's less FIFO stuffing to do but also just generally on longer messages. You will get some overhead setting up two transfers on write then read messages which might offset that but my best guess would be that it'll be negligable on most controllers. It's also just a more accurate representation of what the transfers are actually doing which seems nicer. If there *is* a performance win for doing full duplex messages on some controllers we should probably look at optimizing this in the SPI core since it'll affect a wide range of hardware and we already have some code for forcing full duplex anyway. signature.asc Description: PGP signature
Re: [PATCH] can: mcp251x: convert to half-duplex SPI
On Wed, Feb 26, 2020 at 2:19 AM Marc Kleine-Budde wrote:
>
> On 2/26/20 8:37 AM, Marc Kleine-Budde wrote:
> >> Your right... there is the mcp251x_hw_rx_frame() call that also uses
> >> spi_rx_buf after a synchronous transfer (I didn't see any others).
> >> I'll look at this again.
> >
> > Have you hardware to test your changes? I think the SPI framework would
> > return an -EINVAL in that casethough the return value is sometimes
> > not checked by the driver :/
>
> See https://elixir.bootlin.com/linux/v5.5.6/source/drivers/spi/spi.c#L3413
>
> If you have really have HW with SPI_CONTROLLER_HALF_DUPLEX (a.k.a
> SPI_MASTER_HALF_DUPLEX) restrictions, you need to convert _every_
> mcp251x_spi_trans() call in the driver, as _always_ both rx_buf _and_
> tx_buf are used.
>
Marc,
Sorry for the long delay... I'm finally getting back to this issue.
I'm told by Marvell/Cavium that the OcteonTX SPI hardware does not
support full duplex although I don't see this in any of their errata
or reference manuals. Perhaps someone familiar with the CN81xx/CN80xx
OcteonTX hardware from Marvell/Cavium can weigh in here as I'm not
clear if this limitation is in all hardware that uses the
spi-cavium-thunderx.c driver (I've added Jan to the list who authored
the driver)
As you point out setting SPI_CONTROLLER_HALF_DUPLEX will cause
spi_{sync,async,async_locked} calls to fail with -EINVAL if they have
both a tx and rx buf, so this should be done to help catch these
issues:
diff --git a/drivers/spi/spi-cavium-thunderx.c
b/drivers/spi/spi-cavium-thunderx.c
index fd6b9ca..76fdb94 100644
--- a/drivers/spi/spi-cavium-thunderx.c
+++ b/drivers/spi/spi-cavium-thunderx.c
@@ -64,6 +65,7 @@ static int thunderx_spi_probe(struct pci_dev *pdev,
p->sys_freq = SYS_FREQ_DEFAULT;
dev_info(dev, "Set system clock to %u\n", p->sys_freq);
+ master->flags = SPI_MASTER_HALF_DUPLEX;
master->num_chipselect = 4;
master->mode_bits = SPI_CPHA | SPI_CPOL | SPI_CS_HIGH |
SPI_LSB_FIRST | SPI_3WIRE;
Now, with regards to the mcp251x.c driver you were correct that I was
missing dealing with the full-duplex call from mcp251x_hw_rx_frame()
which indeed was causing data corruption on recieve.
So the following patch to mcp251x.c properly converts mcp251x to half-duplex:
diff --git a/drivers/net/can/spi/mcp251x.c b/drivers/net/can/spi/mcp251x.c
index 5009ff2..016c1e5 100644
--- a/drivers/net/can/spi/mcp251x.c
+++ b/drivers/net/can/spi/mcp251x.c
@@ -290,23 +290,23 @@ static u8 mcp251x_read_reg(struct spi_device *spi, u8 reg)
priv->spi_tx_buf[0] = INSTRUCTION_READ;
priv->spi_tx_buf[1] = reg;
- mcp251x_spi_trans(spi, 3);
- val = priv->spi_rx_buf[2];
+ spi_write_then_read(spi, priv->spi_tx_buf, 2, , 1);
return val;
}
static void mcp251x_read_2regs(struct spi_device *spi, u8 reg, u8 *v1, u8 *v2)
{
+ u8 val[2] = {0};
struct mcp251x_priv *priv = spi_get_drvdata(spi);
priv->spi_tx_buf[0] = INSTRUCTION_READ;
priv->spi_tx_buf[1] = reg;
- mcp251x_spi_trans(spi, 4);
+ spi_write_then_read(spi, priv->spi_tx_buf, 2, val, 2);
- *v1 = priv->spi_rx_buf[2];
- *v2 = priv->spi_rx_buf[3];
+ *v1 = val[0];
+ *v2 = val[1];
}
static void mcp251x_write_reg(struct spi_device *spi, u8 reg, u8 val)
@@ -409,8 +409,9 @@ static void mcp251x_hw_rx_frame(struct spi_device
*spi, u8 *buf,
buf[i] = mcp251x_read_reg(spi, RXBCTRL(buf_idx) + i);
} else {
priv->spi_tx_buf[RXBCTRL_OFF] = INSTRUCTION_READ_RXB(buf_idx);
- mcp251x_spi_trans(spi, SPI_TRANSFER_BUF_LEN);
- memcpy(buf, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN);
+ spi_write_then_read(spi, priv->spi_tx_buf, 1, priv->spi_rx_buf,
+ SPI_TRANSFER_BUF_LEN);
+ memcpy(buf + 1, priv->spi_rx_buf, SPI_TRANSFER_BUF_LEN - 1);
}
}
I do have hardware to test with and without this patch my CN80xx board
with an MCP25625 fails device probing (mcp251x spi0.1: MCP251x didn't
enter in conf mode after reset) because read values are corrupt. With
this patch my the MCP25625 works fine on the CN80xx detecting,
sending, and receiving frames.
Should I be submitting this patch with logic that only does
half-duplex if the spi controller doesn't support it (if
(spi->controller->flags & SPI_CONTROLLER_HALF_DUPLEX)) or is it
acceptable to simply make the driver half-duplex like this for all
cases?
Best Regards,
Tim
